Avoid premature serial timeouts

  On 11/22/2016 11:17 AM, Luis Machado wrote:

> 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?

Sounds good, will do.

> But please fix the formatting according to the gnu coding standards.

Oops, sorry about that. Think I got it now.

> 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.

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.
  

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.