Re: [PATCH] Avoid premature serial timeouts

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