From patchwork Tue Mar 14 14:29:02 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 19572 Received: (qmail 66205 invoked by alias); 14 Mar 2017 14:29:07 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 66189 invoked by uid 89); 14 Mar 2017 14:29:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=fds, smoke, 786, 4994 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 14 Mar 2017 14:29:04 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id EAE488046A; Tue, 14 Mar 2017 14:29:04 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v2EET3fB012404; Tue, 14 Mar 2017 10:29:04 -0400 Subject: Re: [PATCH] PR remote/21188: Fix remote serial timeout To: Gareth McMullin , gdb-patches@sourceware.org References: From: Pedro Alves Message-ID: Date: Tue, 14 Mar 2017 14:29:02 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: 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 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. */