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

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

[Gareth McMullin]

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

The timeout mechanism in ser-unix.c was changed in commit 048094acc.

In do_hardwire_readchar(), the required timeout is broken into 1
second intervals and wait_for() is called.  Before, wait_for() set
VTIME and VMIN so the read would block, but now it uses select() to
block for the specified timeout.  If wait_for() returns
SERIAL_TIMEOUT, do_hardwire_readchar() returns immediately, so the
timeout is always only 1s.

The attached patch will repeatedly call wait_for() until the full
timeout has elapsed.

Gareth

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

2017-02-21 Gareth McMullin  <gareth@blacksphere.co.nz>

	PR remote/21188
	* ser-unix.c (do_hardwire_readchar): Wait for full timeout to elapse.

diff --git a/gdb/ser-unix.c b/gdb/ser-unix.c
index b9e55f0..0a74672 100644
--- a/gdb/ser-unix.c
+++ b/gdb/ser-unix.c
@@ -549,6 +549,18 @@ do_hardwire_readchar (struct serial *scb, int timeout)
       scb->timeout_remaining = (timeout < 0 ? timeout : timeout - delta);
       status = wait_for (scb, delta);

+      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;
+      }
+
       if (status < 0)
 	return status;

@@ -556,21 +568,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.  */

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

[Gareth McMullin]

Ping

On Tue, Feb 21, 2017 at 11:52 AM, Gareth McMullin
<gareth@blacksphere.co.nz> wrote:
> The timeout mechanism in ser-unix.c was changed in commit 048094acc.
>
> In do_hardwire_readchar(), the required timeout is broken into 1
> second intervals and wait_for() is called.  Before, wait_for() set
> VTIME and VMIN so the read would block, but now it uses select() to
> block for the specified timeout.  If wait_for() returns
> SERIAL_TIMEOUT, do_hardwire_readchar() returns immediately, so the
> timeout is always only 1s.
>
> The attached patch will repeatedly call wait_for() until the full
> timeout has elapsed.
>
> Gareth



-- 
Black Sphere Technologies Ltd.

Web: www.blacksphere.co.nz
Mobile: +64 27 777 2182
Tel: +64 9 478 8885
Skype: gareth.mcmullin
LinkedIn: http://nz.linkedin.com/in/gsmcmullin

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

[Simon Marchi]

On 17-02-20 05:52 PM, Gareth McMullin wrote:
> The timeout mechanism in ser-unix.c was changed in commit 048094acc.
> 
> In do_hardwire_readchar(), the required timeout is broken into 1
> second intervals and wait_for() is called.  Before, wait_for() set
> VTIME and VMIN so the read would block, but now it uses select() to
> block for the specified timeout.  If wait_for() returns
> SERIAL_TIMEOUT, do_hardwire_readchar() returns immediately, so the
> timeout is always only 1s.
> 
> The attached patch will repeatedly call wait_for() until the full
> timeout has elapsed.
> 
> Gareth
> 

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.

Simon

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

[Gareth McMullin]

[-- 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.  */

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

[Simon Marchi]

On 2017-03-13 21:02, Gareth McMullin wrote:
> 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.

Thanks for the updated patch.  I just gave it a quick look, and it looks 
good functionally.  Just a little formatting issue.  The four lines:

   else if (timeout_remaining < 0)
     continue;
   else
     return SERIAL_TIMEOUT;

should have one less indent (shift them two spaces to the left).

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

[Gareth McMullin]

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

On Tue, Mar 14, 2017 at 2:40 PM, Simon Marchi <simon.marchi@polymtl.ca> wrote:
> Thanks for the updated patch.  I just gave it a quick look, and it looks
> good functionally.  Just a little formatting issue.  The four lines:
>
>   else if (timeout_remaining < 0)
>     continue;
>   else
>     return SERIAL_TIMEOUT;
>
> should have one less indent (shift them two spaces to the left).

Thank you.  Corrected.

[-- Attachment #2: serial_timeout.patch --]
[-- Type: text/x-patch, Size: 2727 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.  */

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

[Pedro Alves]

On 02/20/2017 10:52 PM, Gareth McMullin wrote:
> The timeout mechanism in ser-unix.c was changed in commit 048094acc.
> 
> In do_hardwire_readchar(), the required timeout is broken into 1
> second intervals and wait_for() is called.  Before, wait_for() set
> VTIME and VMIN so the read would block, but now it uses select() to
> block for the specified timeout.  If wait_for() returns
> SERIAL_TIMEOUT, do_hardwire_readchar() returns immediately, so the
> timeout is always only 1s.
> 
> The attached patch will repeatedly call wait_for() until the full
> timeout has elapsed.
> 

Hmm, since we now always interrupt_select, doesn't this mean we can
finally get rid of do_hardwire_readchar entirely?

I notice that the current ser-unix.c code calls "read" even if wait_for
returns timeout.  That made sense when we used VTIME/VMIN, but by using
select that doesn't seem to make sense any longer.

I've smoke tested this with:

 $ socat -d -d pty,raw,echo=0 pty,raw,echo=0
 2017/03/14 14:08:13 socat[4994] N PTY is /dev/pts/14
 2017/03/14 14:08:13 socat[4994] N PTY is /dev/pts/15
 2017/03/14 14:08:13 socat[4994] N starting data transfer loop with FDs [3,3] and [5,5]
 $ gdbserver /dev/pts/14 PROG 
 $ gdb PROG -ex "tar rem /dev/pts/15"

and then a few continues/ctrl-c's, plus killing gdbserver and socat.
But that doesn't really exercise it completely.

WDYT?

From b4932d1c3dc288e461e5f4514107cb26ff348ae1 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 14 Mar 2017 13:53:23 +0000
Subject: [PATCH] ser_base_read_char

---
 gdb/ser-base.c |  14 ++++--
 gdb/ser-unix.c | 152 +--------------------------------------------------------
 gdb/serial.h   |   5 --
 3 files changed, 11 insertions(+), 160 deletions(-)

diff --git a/gdb/ser-base.c b/gdb/ser-base.c
index 3e10033..e2ac1b6 100644
--- a/gdb/ser-base.c
+++ b/gdb/ser-base.c
@@ -205,6 +205,11 @@ push_event (void *context)
 /* Wait for input on scb, with timeout seconds.  Returns 0 on success,
    otherwise SERIAL_TIMEOUT or SERIAL_ERROR.  */
 
+/* NOTE: Some of the code below is dead.  The only possible values of
+   the TIMEOUT parameter are ONE and ZERO.  Consequently the code that
+   tries to handle the possibility of an overflowed timer is
+   unnecessary.  */
+
 static int
 ser_base_wait_for (struct serial *scb, int timeout)
 {
@@ -308,10 +313,11 @@ ser_base_read_error_fd (struct serial *scb, int close_fd)
     }
 }
 
-/* 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 -2 if timeout expired, EOF if line dropped
-   dead, or -3 for any other error (see errno in that case).  */
+/* 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, SERIAL_EOF if line dropped dead, or SERIAL_ERROR
+   for any other error (see errno in that case).  */
 
 static int
 do_ser_base_readchar (struct serial *scb, int timeout)
diff --git a/gdb/ser-unix.c b/gdb/ser-unix.c
index b9e55f0..5c93891 100644
--- a/gdb/ser-unix.c
+++ b/gdb/ser-unix.c
@@ -78,9 +78,6 @@ struct hardwire_ttystate
 
 static int hardwire_open (struct serial *scb, const char *name);
 static void hardwire_raw (struct serial *scb);
-static int wait_for (struct serial *scb, int timeout);
-static int hardwire_readchar (struct serial *scb, int timeout);
-static int do_hardwire_readchar (struct serial *scb, int timeout);
 static int rate_to_code (int rate);
 static int hardwire_setbaudrate (struct serial *scb, int rate);
 static int hardwire_setparity (struct serial *scb, int parity);
@@ -441,155 +438,11 @@ 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));
 }
 
-/* Wait for input on scb, with timeout seconds.  Returns 0 on success,
-   otherwise SERIAL_TIMEOUT or SERIAL_ERROR.  */
-
-/* FIXME: cagney/1999-09-16: Don't replace this with the equivalent
-   ser_base*() until the old TERMIOS/SGTTY/... timer code has been
-   flushed. .  */
-
-/* NOTE: cagney/1999-09-30: Much of the code below is dead.  The only
-   possible values of the TIMEOUT parameter are ONE and ZERO.
-   Consequently all the code that tries to handle the possability of
-   an overflowed timer is unnecessary.  */
-
-static int
-wait_for (struct serial *scb, int timeout)
-{
-  while (1)
-    {
-      struct timeval tv;
-      fd_set readfds;
-      int numfds;
-
-      /* NOTE: Some OS's can scramble the READFDS when the select()
-         call fails (ex the kernel with Red Hat 5.2).  Initialize all
-         arguments before each call.  */
-
-      tv.tv_sec = timeout;
-      tv.tv_usec = 0;
-
-      FD_ZERO (&readfds);
-      FD_SET (scb->fd, &readfds);
-
-      QUIT;
-
-      if (timeout >= 0)
-	numfds = interruptible_select (scb->fd + 1, &readfds, 0, 0, &tv);
-      else
-	numfds = interruptible_select (scb->fd + 1, &readfds, 0, 0, 0);
-
-      if (numfds == -1 && errno == EINTR)
-	continue;
-      else if (numfds == -1)
-	return SERIAL_ERROR;
-      else if (numfds == 0)
-	return SERIAL_TIMEOUT;
-
-      return 0;
-    }
-}
-
-/* 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).  */
-
-/* FIXME: cagney/1999-09-16: Don't replace this with the equivalent
-   ser_base*() until the old TERMIOS/SGTTY/... timer code has been
-   flushed.  */
-
-/* NOTE: cagney/1999-09-16: This function is not identical to
-   ser_base_readchar() as part of replacing it with ser_base*()
-   merging will be required - this code handles the case where read()
-   times out due to no data while ser_base_readchar() doesn't expect
-   that.  */
-
-static int
-do_hardwire_readchar (struct serial *scb, int timeout)
-{
-  int status, delta;
-  int detach = 0;
-
-  if (timeout > 0)
-    timeout++;
-
-  /* We have to be able to keep the GUI alive here, so we break the
-     original timeout into steps of 1 second, running the "keep the
-     GUI alive" hook each time through the loop.
-
-     Also, timeout = 0 means to poll, so we just set the delta to 0,
-     so we will only go through the loop once.  */
-
-  delta = (timeout == 0 ? 0 : 1);
-  while (1)
-    {
-
-      /* 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
-         quickly as possible.  It is not safe to touch scb, since
-         someone else might have freed it.  The
-         deprecated_ui_loop_hook signals that we should exit by
-         returning 1.  */
-
-      if (deprecated_ui_loop_hook)
-	detach = deprecated_ui_loop_hook (0);
-
-      if (detach)
-	return SERIAL_TIMEOUT;
-
-      scb->timeout_remaining = (timeout < 0 ? timeout : timeout - delta);
-      status = wait_for (scb, delta);
-
-      if (status < 0)
-	return status;
-
-      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.  */
-	}
-
-      scb->bufcnt = status;
-      scb->bufcnt--;
-      scb->bufp = scb->buf;
-      return *scb->bufp++;
-    }
-}
-
-static int
-hardwire_readchar (struct serial *scb, int timeout)
-{
-  return generic_readchar (scb, timeout, do_hardwire_readchar);
-}
-
-
 #ifndef B19200
 #define B19200 EXTA
 #endif
@@ -882,10 +735,7 @@ static const struct serial_ops hardwire_ops =
   hardwire_open,
   hardwire_close,
   NULL,
-  /* FIXME: Don't replace this with the equivalent ser_base*() until
-     the old TERMIOS/SGTTY/... timer code has been flushed.  cagney
-     1999-09-16.  */
-  hardwire_readchar,
+  ser_base_readchar,
   ser_base_write,
   hardwire_flush_output,
   hardwire_flush_input,
diff --git a/gdb/serial.h b/gdb/serial.h
index cf4e659..b39cc33 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.  */
-- 
2.5.5

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

[Pedro Alves]

On 03/14/2017 02:29 PM, Pedro Alves wrote:
> On 02/20/2017 10:52 PM, Gareth McMullin wrote:
>> The timeout mechanism in ser-unix.c was changed in commit 048094acc.
>>
>> In do_hardwire_readchar(), the required timeout is broken into 1
>> second intervals and wait_for() is called.  Before, wait_for() set
>> VTIME and VMIN so the read would block, but now it uses select() to
>> block for the specified timeout.  If wait_for() returns
>> SERIAL_TIMEOUT, do_hardwire_readchar() returns immediately, so the
>> timeout is always only 1s.
>>
>> The attached patch will repeatedly call wait_for() until the full
>> timeout has elapsed.
>>
> 
> Hmm, since we now always interrupt_select, doesn't this mean we can
> finally get rid of do_hardwire_readchar entirely?

[...]

> WDYT?

I went ahead and pushed it in, as below.  Let me know if something
is still odd.  Thanks!

From 9bcbdca808b5f9fec6217d20bd4b48a56008c460 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 17 Mar 2017 16:08:12 +0000
Subject: [PATCH] PR remote/21188: Fix remote serial timeout

As Gareth McMullin <gareth@blacksphere.co.nz> reports at
<https://sourceware.org/ml/gdb-patches/2017-02/msg00560.html>, the
timeout mechanism in ser-unix.c was broken by commit 048094acc
("target remote: Don't rely on immediate_quit (introduce quit
handlers)").

Instead of applying a local fix, and since we now finally always use
interrupt_select [1], let's get rid of hardwire_readchar entirely, and
use ser_base_readchar instead, which has similar timeout handling,
except for the bug.

Smoke tested with:

 $ socat -d -d pty,raw,echo=0 pty,raw,echo=0
 2017/03/14 14:08:13 socat[4994] N PTY is /dev/pts/14
 2017/03/14 14:08:13 socat[4994] N PTY is /dev/pts/15
 2017/03/14 14:08:13 socat[4994] N starting data transfer loop with FDs [3,3] and [5,5]
 $ gdbserver /dev/pts/14 PROG
 $ gdb PROG -ex "tar rem /dev/pts/15"

and then a few continues/ctrl-c's, plus killing gdbserver and socat.

[1] - See FIXME comments being removed.

gdb/ChangeLog:
2017-03-17  Pedro Alves  <palves@redhat.com>

	PR remote/21188
	* ser-base.c (ser_base_wait_for): Add comment.
	(do_ser_base_readchar): Improve comment based on the ser-unix.c's
	version.
	* ser-unix.c (hardwire_raw): Remove reference to
	scb->current_timeout.
	(wait_for, do_hardwire_readchar, hardwire_readchar): Delete.
	(hardwire_ops): Install ser_base_readchar instead of
	hardwire_readchar.
	* serial.h (struct serial) <current_timeout, timeout_remaining>:
	Remove fields.
---
 gdb/ChangeLog  |  14 ++++++
 gdb/ser-base.c |  14 ++++--
 gdb/ser-unix.c | 152 +--------------------------------------------------------
 gdb/serial.h   |   5 --
 4 files changed, 25 insertions(+), 160 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 6d81cf5..ca7a49e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,17 @@
+2017-03-17  Pedro Alves  <palves@redhat.com>
+
+	PR remote/21188
+	* ser-base.c (ser_base_wait_for): Add comment.
+	(do_ser_base_readchar): Improve comment based on the ser-unix.c's
+	version.
+	* ser-unix.c (hardwire_raw): Remove reference to
+	scb->current_timeout.
+	(wait_for, do_hardwire_readchar, hardwire_readchar): Delete.
+	(hardwire_ops): Install ser_base_readchar instead of
+	hardwire_readchar.
+	* serial.h (struct serial) <current_timeout, timeout_remaining>:
+	Remove fields.
+
 2017-03-17  Jonah Graham  <jonah@kichwacoders.com>
 
 	PR gdb/19637
diff --git a/gdb/ser-base.c b/gdb/ser-base.c
index 3e10033..790cb1b 100644
--- a/gdb/ser-base.c
+++ b/gdb/ser-base.c
@@ -205,6 +205,11 @@ push_event (void *context)
 /* Wait for input on scb, with timeout seconds.  Returns 0 on success,
    otherwise SERIAL_TIMEOUT or SERIAL_ERROR.  */
 
+/* NOTE: Some of the code below is dead.  The only possible values of
+   the TIMEOUT parameter are ONE and ZERO.  OTOH, we should probably
+   get rid of the deprecated_ui_loop_hook call in do_ser_base_readchar
+   instead and support infinite time outs here.  */
+
 static int
 ser_base_wait_for (struct serial *scb, int timeout)
 {
@@ -308,10 +313,11 @@ ser_base_read_error_fd (struct serial *scb, int close_fd)
     }
 }
 
-/* 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 -2 if timeout expired, EOF if line dropped
-   dead, or -3 for any other error (see errno in that case).  */
+/* 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, SERIAL_EOF if line dropped dead, or SERIAL_ERROR
+   for any other error (see errno in that case).  */
 
 static int
 do_ser_base_readchar (struct serial *scb, int timeout)
diff --git a/gdb/ser-unix.c b/gdb/ser-unix.c
index b9e55f0..5c93891 100644
--- a/gdb/ser-unix.c
+++ b/gdb/ser-unix.c
@@ -78,9 +78,6 @@ struct hardwire_ttystate
 
 static int hardwire_open (struct serial *scb, const char *name);
 static void hardwire_raw (struct serial *scb);
-static int wait_for (struct serial *scb, int timeout);
-static int hardwire_readchar (struct serial *scb, int timeout);
-static int do_hardwire_readchar (struct serial *scb, int timeout);
 static int rate_to_code (int rate);
 static int hardwire_setbaudrate (struct serial *scb, int rate);
 static int hardwire_setparity (struct serial *scb, int parity);
@@ -441,155 +438,11 @@ 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));
 }
 
-/* Wait for input on scb, with timeout seconds.  Returns 0 on success,
-   otherwise SERIAL_TIMEOUT or SERIAL_ERROR.  */
-
-/* FIXME: cagney/1999-09-16: Don't replace this with the equivalent
-   ser_base*() until the old TERMIOS/SGTTY/... timer code has been
-   flushed. .  */
-
-/* NOTE: cagney/1999-09-30: Much of the code below is dead.  The only
-   possible values of the TIMEOUT parameter are ONE and ZERO.
-   Consequently all the code that tries to handle the possability of
-   an overflowed timer is unnecessary.  */
-
-static int
-wait_for (struct serial *scb, int timeout)
-{
-  while (1)
-    {
-      struct timeval tv;
-      fd_set readfds;
-      int numfds;
-
-      /* NOTE: Some OS's can scramble the READFDS when the select()
-         call fails (ex the kernel with Red Hat 5.2).  Initialize all
-         arguments before each call.  */
-
-      tv.tv_sec = timeout;
-      tv.tv_usec = 0;
-
-      FD_ZERO (&readfds);
-      FD_SET (scb->fd, &readfds);
-
-      QUIT;
-
-      if (timeout >= 0)
-	numfds = interruptible_select (scb->fd + 1, &readfds, 0, 0, &tv);
-      else
-	numfds = interruptible_select (scb->fd + 1, &readfds, 0, 0, 0);
-
-      if (numfds == -1 && errno == EINTR)
-	continue;
-      else if (numfds == -1)
-	return SERIAL_ERROR;
-      else if (numfds == 0)
-	return SERIAL_TIMEOUT;
-
-      return 0;
-    }
-}
-
-/* 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).  */
-
-/* FIXME: cagney/1999-09-16: Don't replace this with the equivalent
-   ser_base*() until the old TERMIOS/SGTTY/... timer code has been
-   flushed.  */
-
-/* NOTE: cagney/1999-09-16: This function is not identical to
-   ser_base_readchar() as part of replacing it with ser_base*()
-   merging will be required - this code handles the case where read()
-   times out due to no data while ser_base_readchar() doesn't expect
-   that.  */
-
-static int
-do_hardwire_readchar (struct serial *scb, int timeout)
-{
-  int status, delta;
-  int detach = 0;
-
-  if (timeout > 0)
-    timeout++;
-
-  /* We have to be able to keep the GUI alive here, so we break the
-     original timeout into steps of 1 second, running the "keep the
-     GUI alive" hook each time through the loop.
-
-     Also, timeout = 0 means to poll, so we just set the delta to 0,
-     so we will only go through the loop once.  */
-
-  delta = (timeout == 0 ? 0 : 1);
-  while (1)
-    {
-
-      /* 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
-         quickly as possible.  It is not safe to touch scb, since
-         someone else might have freed it.  The
-         deprecated_ui_loop_hook signals that we should exit by
-         returning 1.  */
-
-      if (deprecated_ui_loop_hook)
-	detach = deprecated_ui_loop_hook (0);
-
-      if (detach)
-	return SERIAL_TIMEOUT;
-
-      scb->timeout_remaining = (timeout < 0 ? timeout : timeout - delta);
-      status = wait_for (scb, delta);
-
-      if (status < 0)
-	return status;
-
-      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.  */
-	}
-
-      scb->bufcnt = status;
-      scb->bufcnt--;
-      scb->bufp = scb->buf;
-      return *scb->bufp++;
-    }
-}
-
-static int
-hardwire_readchar (struct serial *scb, int timeout)
-{
-  return generic_readchar (scb, timeout, do_hardwire_readchar);
-}
-
-
 #ifndef B19200
 #define B19200 EXTA
 #endif
@@ -882,10 +735,7 @@ static const struct serial_ops hardwire_ops =
   hardwire_open,
   hardwire_close,
   NULL,
-  /* FIXME: Don't replace this with the equivalent ser_base*() until
-     the old TERMIOS/SGTTY/... timer code has been flushed.  cagney
-     1999-09-16.  */
-  hardwire_readchar,
+  ser_base_readchar,
   ser_base_write,
   hardwire_flush_output,
   hardwire_flush_input,
diff --git a/gdb/serial.h b/gdb/serial.h
index cf4e659..b39cc33 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.  */
-- 
2.5.5