process_read_data() should not keep retrying

[hackerb9]

Currently process_read_data() will keep retrying on EAGAIN until more bytes have been read than the read buffer can contain (hardcoded to 1024).

This is unnecessary because process_read_data is already called in a loop and actually harmful because the program will hang, waiting for data that will never show up.

I fixed it in my fork by making these changes:

@@ -538,10 +682,11 @@ static unsigned char next_count_value(unsigned char c)
 
 static void process_read_data(void)
 {
-	unsigned char rb[1024];
+	const int RBSIZE = 1024;
+	unsigned char rb[RBSIZE];
 	int actual_read_count = 0;
-	while (actual_read_count < 1024) {
-		int c = read(_fd, &rb, sizeof(rb));
+	while (actual_read_count < RBSIZE) {
+		int c = read(_fd, &rb, (RBSIZE-actual_read_count));
 		if (c > 0) {
 			if (_cl_rx_dump) {
 				if (_cl_rx_dump_ascii)
@@ -569,17 +714,13 @@ static void process_read_data(void)
 			}
 			_read_count += c;
 			actual_read_count += c;
-		} else if (errno) {
-			if (errno != EAGAIN) {
-				perror("read failed");
-			}
-			continue; // Retry the read
 		} else {
-		    break;
+			break; // Do not continue! We already loop on reading.
 		}
 	}
 	if (_cl_rx_detailed) {
-		printf("Read %d bytes\n", actual_read_count);
+		printf("Read %d bytes %s\n", actual_read_count,
+		       (actual_read_count < RBSIZE)?"":"(buffer limit)");
 	}
 }

Note that this also fixes a buffer overflow because each read() previously tried to add 1024 more bytes to the 1024 byte buffer. Now, it requests only as many bytes as are available in the buffer.

[cbrake]

do you want to open PRs for these improvements?

[hackerb9]

I wasn't intending to, but if they are actually useful to you, I can look into it. The reason I didn't was that I made the changes outside of git and so all the issues I fixed are just one huge patch.

[andy-shev]

This is definitely not the best approach as you removed the cases when returned error code is not EAGAIN. Have you had a chance to read man 2 read in the returned errors section?

[hackerb9]

Oh jeez. Thanks for pointing out that I removed the error checking. While I can see why you asked the question, yes, I have read read(2) and understand it. It was a silly mistake.

Not a defense, but an explanation: I had not intended to make a pull request originally. For a brief time, I had need of a serial port tester. I found linux-serial-test, saw it was broken, and got it working. Since it was useful to me, I reported the issues, hoping to contribute a little back to the project.

As for "not being the best approach", you're not kidding! There's an even stronger reason why my code is wrong. I had presumed that the magic number "1024" being used twice in a row meant something: It was both the buffer size and the number of bytes at which the routine stopped reading. However, it turns out that was completely a coincidence. So, the "fix" I made to the while loop to ensure it didn't read more than 1024 bytes is useless.

I believe the correct solution is to revert 7fd1057 which caused the routine to hang in the first place. That is, don't fix the while loop, remove it.

[andy-shev]

Oh jeez. Thanks for pointing out that I removed the error checking. While I can see why you asked the question, yes, I have read read(2) and understand it. It was a silly mistake.

Not a defense, but an explanation: I had not intended to make a pull request originally. For a brief time, I had need of a serial port tester. I found linux-serial-test, saw it was broken, and got it working. Since it was useful to me, I reported the issues, hoping to contribute a little back to the project.

As for "not being the best approach", you're not kidding! There's an even stronger reason why my code is wrong. I had presumed that the magic number "1024" being used twice in a row meant something: It was both the buffer size and the number of bytes at which the routine stopped reading. However, it turns out that was completely a coincidence. So, the "fix" I made to the while loop to ensure it didn't read more than 1024 bytes is useless.

I believe the correct solution is to revert 7fd1057 which caused the routine to hang in the first place. That is, don't fix the while loop, remove it.

I leave it to Cliff to decide.