Re: [PATCH] PR remote/21188: Fix remote serial timeout

[Gareth McMullin <gareth@blacksphere.co.nz>]

[-- Attachment #1: Type: text/plain, Size: 833 bytes --]

On Fri, Mar 3, 2017 at 5:14 AM, Simon Marchi <simon.marchi@ericsson.com> wrote:
> I think I understand the problem you describe by inspecting the code.  However,
> I have some difficulty understanding the current and proposed code, so I can't
> say if the patch looks correct.  It just looks more complicated than necessary.
>
> For example, what's the point of the timeout_remaining field in struct serial?  It
> seems to ever only be used in this function.  If we can remove it, it will be one
> less thing to consider.  We can probably have just the timeout variable that we
> decrement until it's done.

Thank you, Simon.  I only made the smallest change needed to fix the
problem.  I've attached an updated patch
to replace the timeout_remaining field with a local variable, and
remove the unused current_timeout field.

Gareth

[-- Attachment #2: serial_timeout.patch --]
[-- Type: text/x-patch, Size: 2735 bytes --]

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 608501b..111cf4d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2017-03-14 Gareth McMullin  <gareth@blacksphere.co.nz>
+
+	PR remote/21188
+	* ser-unix.c (do_hardwire_readchar): Wait for full timeout to elapse.
+	* serial.h (serial_t): Remove fields current_timeout and timeout_remaining.
+
 2017-03-14  Pedro Alves  <palves@redhat.com>

 	* cp-name-parser.y (cp_demangled_name_to_comp): Update comment.
diff --git a/gdb/ser-unix.c b/gdb/ser-unix.c
index b9e55f0..7f73af8 100644
--- a/gdb/ser-unix.c
+++ b/gdb/ser-unix.c
@@ -441,8 +441,6 @@ hardwire_raw (struct serial *scb)
   state.sgttyb.sg_flags &= ~(CBREAK | ECHO);
 #endif

-  scb->current_timeout = 0;
-
   if (set_tty_state (scb, &state))
     fprintf_unfiltered (gdb_stderr, "set_tty_state failed: %s\n",
 			safe_strerror (errno));
@@ -546,9 +544,21 @@ do_hardwire_readchar (struct serial *scb, int timeout)
       if (detach)
 	return SERIAL_TIMEOUT;

-      scb->timeout_remaining = (timeout < 0 ? timeout : timeout - delta);
+      int timeout_remaining = (timeout < 0 ? timeout : timeout - delta);
       status = wait_for (scb, delta);

+      if (status == SERIAL_TIMEOUT) {
+	if (timeout_remaining > 0)
+	  {
+	    timeout = timeout_remaining;
+	    continue;
+	  }
+	  else if (timeout_remaining < 0)
+	    continue;
+	  else
+	    return SERIAL_TIMEOUT;
+      }
+
       if (status < 0)
 	return status;

@@ -556,21 +566,7 @@ do_hardwire_readchar (struct serial *scb, int timeout)

       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)
+	  if (errno == EINTR)
 	    continue;
 	  else
 	    return SERIAL_ERROR;	/* Got an error from read.  */
diff --git a/gdb/serial.h b/gdb/serial.h
index cf4e659..2900507 100644
--- a/gdb/serial.h
+++ b/gdb/serial.h
@@ -250,11 +250,6 @@ struct serial
 				   buffer.  -ve for sticky errors.  */
     unsigned char *bufp;	/* Current byte */
     unsigned char buf[BUFSIZ];	/* Da buffer itself */
-    int current_timeout;	/* (ser-unix.c termio{,s} only), last
-				   value of VTIME */
-    int timeout_remaining;	/* (ser-unix.c termio{,s} only), we
-				   still need to wait for this many
-				   more seconds.  */
     struct serial *next;	/* Pointer to the next `struct serial *' */
     int debug_p;		/* Trace this serial devices operation.  */
     int async_state;		/* Async internal state.  */