This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Avoid premature serial timeouts
- From: Luis Machado <lgustavo at codesourcery dot com>
- To: Michael Daniels <mdaniels at blackberry dot com>, "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Tue, 22 Nov 2016 10:17:08 -0600
- Subject: Re: [PATCH] Avoid premature serial timeouts
- Authentication-results: sourceware.org; auth=none
- References: <303AEF3B642EDD4B994EA21E8DEFDC5E6F94D5@XMB126CNC.rim.net>
- Reply-to: Luis Machado <lgustavo at codesourcery dot com>
Hi,
On 11/18/2016 03:36 PM, Michael Daniels wrote:
I noticed that when calling serial_readchar() with a large timeout
(or -1), and there is no data to read for > 1s then it will prematurely
return SERIAL_TIMEOUT.
I narrowed it down to do_hardwire_readchar() calling wait_for() with a
timeout of 1, and returning immediately if there was an error/timeout.
I have moved some lines around to capture what I believe was the original
intent.
Thoughts?
The function indeed seems to be buggy, but i think it needs a few more
adjustments.
The function is abusing the "status" variable a little bit. It holds the
status for wait_for (...) and also holds the number of bytes read, which
is slightly confusing and error-prone. We should probably have separate
variables for those, so "status" and "byte_count" or some other name?
diff --git a/gdb/ser-unix.c b/gdb/ser-unix.c
index 2e7b1b4..9bd45f6 100644
--- a/gdb/ser-unix.c
+++ b/gdb/ser-unix.c
@@ -549,32 +549,23 @@ do_hardwire_readchar (struct serial *scb, int timeout)
scb->timeout_remaining = (timeout < 0 ? timeout : timeout - delta);
status = wait_for (scb, delta);
- if (status < 0)
+ if (status == SERIAL_TIMEOUT) {
+ if (scb->timeout_remaining > 0) {
+ timeout = scb->timeout_remaining;
+ continue;
+ } else if (scb->timeout_remaining < 0)
+ continue;
+ else
+ return SERIAL_TIMEOUT;
+ } else if (status < 0)
return status;
I think moving this check upwards is sane, so we can check for timeouts
and errors.
But please fix the formatting according to the gnu coding standards. The
curly braces should be on the next line and on its own without code
following it.
I wonder if the check for scb->timeout_remaining being > 0 and < 0 could
be folded into a single statement that just updates timeout?
if (scb->timeout_remaining != 0)
timeout = scb->timeout_remaining;
else
return SERIAL_TIMEOUT;
Does it make sense? We will update timeout anyway and if timeout was
originally negative, and it will continue to be negative with the
update. If it reached 0, then we return.
status = read (scb->fd, scb->buf, BUFSIZ);
- if (status <= 0)
- {
- if (status == 0)
- {
- /* Zero characters means timeout (it could also be EOF, but
- we don't (yet at least) distinguish). */
- if (scb->timeout_remaining > 0)
- {
- timeout = scb->timeout_remaining;
- continue;
- }
- else if (scb->timeout_remaining < 0)
- continue;
- else
- return SERIAL_TIMEOUT;
- }
- else if (errno == EINTR)
- continue;
- else
- return SERIAL_ERROR; /* Got an error from read. */
- }
+ if (status < 0 && errno == EINTR)
+ continue;
+ else if (status <= 0)
+ return SERIAL_ERROR; /* Got an error from read. */
It looks like the new code doesn't behave like the old one. The old code
returned SERIAL_ERROR if "status < 0 && errno != EINTR)". The new code
returns if "status <= 0 && errno != EINTR)", but status == 0 is
considered a timeout.
It sounds like we should still check for a byte_count == 0 and handle a
timeout situation, where we continue to wait for data until an error
happens either in wait_for (...) or read (...)?
So...
status = wait_for (...)
check for timeout
byte_count = read (...)
check_for_timeout
What do you think?