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] PR remote/21188: Fix remote serial timeout


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



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