This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Avoid premature serial timeouts


On 11/23/2016 11:00 AM, Michael Daniels wrote:
On 11/22/2016 11:17 AM, Luis Machado wrote:

> 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.
Yup, that's a good point. I also moved the the timeout_remaining check
up a bit so the additional return could be removed. Close to what you
had in mind?

>>        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 was considered a timeout before because it didn't distinguish between
EOF and a timeout. With the code moved around, the timeout case is
handled before the read(). My understanding is that if we made it to the
read() then we should be able to call without blocking. In this case a
return of 0 would indicate EOF.

True. The original timeout check may have been misplaced.


My first reaction was to return SERIAL_ERROR in that case, but looking
again I see SERIAL_EOF, which would probably make more sense here.

Here is a new patch, hopefully a bit better now.

diff --git a/gdb/ser-unix.c b/gdb/ser-unix.c
index 2e7b1b4..68ed24e 100644
--- a/gdb/ser-unix.c
+++ b/gdb/ser-unix.c
@@ -500,8 +500,8 @@ wait_for (struct serial *scb, int timeout)
 /* Read a character with user-specified timeout.  TIMEOUT is number of
    seconds to wait, or -1 to wait forever.  Use timeout of 0 to effect
    a poll.  Returns char if successful.  Returns SERIAL_TIMEOUT if
-   timeout expired, EOF if line dropped dead, or SERIAL_ERROR for any
-   other error (see errno in that case).  */
+   timeout expired, SERIAL_EOF if line dropped dead, or SERIAL_ERROR
+   for any other error (see errno in that case).  */

 /* FIXME: cagney/1999-09-16: Don't replace this with the equivalent
    ser_base*() until the old TERMIOS/SGTTY/... timer code has been
@@ -516,7 +516,7 @@ wait_for (struct serial *scb, int timeout)
 static int
 do_hardwire_readchar (struct serial *scb, int timeout)
 {
-  int status, delta;
+  int delta;
   int detach = 0;

   if (timeout > 0)
@@ -532,6 +532,8 @@ do_hardwire_readchar (struct serial *scb, int timeout)
   delta = (timeout == 0 ? 0 : 1);
   while (1)
     {
+      int status;
+      ssize_t byte_count;

       /* N.B. The UI may destroy our world (for instance by calling
          remote_stop,) in which case we want to get out of here as
@@ -549,34 +551,27 @@ 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 && scb->timeout_remaining != 0)
+	{
+	  timeout = scb->timeout_remaining;
+	  continue;
+	}
+      else if (status < 0)
 	return status;

-      status = read (scb->fd, scb->buf, BUFSIZ);
+      byte_count = read (scb->fd, scb->buf, BUFSIZ);

-      if (status <= 0)
+      if (byte_count == 0)
+	return SERIAL_EOF;
+      else if (byte_count < 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)
+	  if (errno == EINTR)
 	    continue;
-	  else
+          else
 	    return SERIAL_ERROR;	/* Got an error from read.  */
 	}

-      scb->bufcnt = status;
+      scb->bufcnt = byte_count;
       scb->bufcnt--;
       scb->bufp = scb->buf;
       return *scb->bufp++;


Thanks. The new patch looks good to me. You'll need a ChangeLog entry for this as well as maintainer approval to commit (if OK). One should chime in soon.

Did you exercise this patch against gdb/gdbserver with the testsuite?

make check-gdb RUNTESTFLAGS="--target_board native-gdbserver"

On a side note, your mail agent seems to be inserting strange characters for spaces and '='. See here: https://sourceware.org/cgi-bin/get-raw-msg?listname=gdb-patches&date=2016-11&msgid=303AEF3B642EDD4B994EA21E8DEFDC5E6FEA93%40XMB126CNC.rim.net.

As a consequence, your patch doesn't apply when fetched from the mailing list archive.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]