Date: Tue, 11 Sep 2012 18:10:15 +0200 From: Steffen "Daode" Nurpmeso <sdaoden@gmail.com> To: Mark Johnston <markjdb@gmail.com> Cc: jilles@FreeBSD.org, freebsd-bugs@FreeBSD.org, pfg@FreeBSD.org, eadler@FreeBSD.org, bug-followup@FreeBSD.org Subject: Re: bin/169773: sh(1): Resizing causes /bin/sh to repeat edit operations Message-ID: <504f6267.yLNFTfIZigyaYVMofL5rUv8U@dietcurd.wild-life.local> In-Reply-To: <504dffa3.fS2d0si9erBWOIHKQIFk0edM@dietcurd.wild-life.local> References: <201209061926.q86JQwAC087821@freefall.freebsd.org> <504b67f8.hU%2BoKTUxDdq8oHD59qxE1DdF@dietcurd.wild-life.local> <20120908212944.GA39382@raichu.mark-home> <504dffa3.fS2d0si9erBWOIHKQIFk0edM@dietcurd.wild-life.local>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format. --=_504f6267.iUCe6UiHu0SzYFl1a67qpbTPwc33pf/H4idnoT7GkXfIKRja Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline |Mark Johnston <markjdb@gmail.com> wrote: | ||On Sat, Sep 08, 2012 at 05:44:56PM +0200, Steffen Daode Nurpmeso wrote: ||> |Synopsis: sh(1): Resizing causes /bin/sh to repeat edit operations ||> | ||> |http://www.freebsd.org/cgi/query-pr.cgi?pr=169773 ||> | [.] ||> It's a rather quick first diff for editline(3), i have no more | [.] || ||I took a closer look at the patch... I think the errno handling is ||mostly ok, except for a couple of places in el_gets() where the return ||value of read_char() isn't stored, and the code ends up looking at an ||uninitialized variable. The attached patch is your patch + the fix for ||this. || ||> I *think* it effectively results in editline(3) behaving the way ||> it is supposed to work (retrying once after a whatever signal, ||> then failing for a second one). Since el_gets() now fails (as it ||> is supposed to), sh(1) will behave wrong in that the current line | [.] ||> ||> It would be better if editline(3) could be configured to simply ||> restart upon EINTR, or to fixate that behaviour (for FreeBSD)? ||> I don't think it is acceptable to loose a line of user content due ||> to a simple resize? ||> So long and ciao, || ||Maybe we need a new option for el_set() which sets a flag in ||el->el_signal that determines whether various functions return on EINTR. ||libfetch for example has fetchRestartCalls for this purpose. It's not ||really clear to me why anyone would want EINTR as an error and return ||though. | |I have implemented a EL_READRESTART option for editline(3), which [.] | ||-Mark I have posted some NetBSD problem reports and some of the patches have been accepted upstream and are already committed. [1][2][3] And upstream editline.3 already documented errno behaviour for el_gets() (not for el_getc(), yet posted a PR for that [4]). It however turned out that fixing the behaviour is even more difficult than i thought yesterday, and the issue is also not completed upstream. (But at least someone who really knows is thinking about it from now on.) So i do attach the current state (less anything which will come in from upstream anyway), including the new restart patch, for which i've also posted a PR upstream [5], since i think it is a useful flag to have. (Though upstream editline(3) does do some EINTR. I also saw pfg@'s commit on that itm.) Anyway - this new restart patch rather applies 1:1 on upstream, too, and it really seems to fix the problem now. It is anyway better than the patch from yesterday, just in case you use that. Ciao, --steffen [1] http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=46935 [2] http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=46941 [3] http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=46942 [4] http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=46945 [5] http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=46943 --=_504f6267.iUCe6UiHu0SzYFl1a67qpbTPwc33pf/H4idnoT7GkXfIKRja Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0001-Fix-editline-3-char-read-and-errno-code-flow.patch" >From e889bca8dc1fe7ddb95651ecbe98ad05bd59e22e Mon Sep 17 00:00:00 2001 Message-Id: <e889bca8dc1fe7ddb95651ecbe98ad05bd59e22e.1347378617.git.sdaoden@gmail.com> From: "Steffen \"Daode\" Nurpmeso" <sdaoden@gmail.com> Date: Mon, 10 Sep 2012 15:50:58 +0200 Subject: [PATCH 1/3] Fix editline(3) char read and errno code flow The reading call chain failed to initialize local variables. >From Mark Johnston (markjdb AT gmail DOT com). A return value from deeper in the chain was reused without localizing the meaning, which resulted in misinterpretation later on. And the tracking of errno in EditLine::el_errno, and vice versa, was also fixed. All this resulted in the requirement for a different way to control the edit loop, fixed by introduction of the new enum rcmd. With help from Christos Zoulas (christos AT zoulas DOT com). --- lib/libedit/read.c | 53 +++++++++++++++++++++++++++++++-------------------- 1 files changed, 32 insertions(+), 21 deletions(-) diff --git a/lib/libedit/read.c b/lib/libedit/read.c index 7d7f54b..0880b5c 100644 --- a/lib/libedit/read.c +++ b/lib/libedit/read.c @@ -49,13 +49,17 @@ __FBSDID("$FreeBSD$"); #include <stdlib.h> #include "el.h" -#define OKCMD -1 - -private int read__fixio(int, int); -private int read_preread(EditLine *); -private int read_char(EditLine *, char *); -private int read_getcmd(EditLine *, el_action_t *, char *); -private void read_pop(c_macro_t *); +enum rcmd { + OKCMD = -1, + EOFCMD = 0, + ERRCMD = 1 +}; + +private int read__fixio(int, int); +private int read_preread(EditLine *); +private int read_char(EditLine *, char *); +private enum rcmd read_getcmd(EditLine *, el_action_t *, char *); +private void read_pop(c_macro_t *); /* read_init(): * Initialize the read stuff @@ -227,9 +231,9 @@ el_push(EditLine *el, const char *str) /* read_getcmd(): - * Return next command from the input stream. + * Get next command from the input stream. */ -private int +private enum rcmd read_getcmd(EditLine *el, el_action_t *cmdnum, char *ch) { el_action_t cmd; @@ -238,8 +242,7 @@ read_getcmd(EditLine *el, el_action_t *cmdnum, char *ch) el->el_errno = 0; do { if ((num = el_getc(el, ch)) != 1) { /* if EOF or error */ - el->el_errno = num == 0 ? 0 : errno; - return (num); + return (num < 0 ? ERRCMD : EOFCMD); } #ifdef KANJI @@ -294,16 +297,18 @@ read_char(EditLine *el, char *cp) again: el->el_signal->sig_no = 0; - while ((num_read = read(el->el_infd, cp, 1)) == -1) { + while ((num_read = read(el->el_infd, cp, 1)) < 0) { + int e = errno; if (el->el_signal->sig_no == SIGCONT) { sig_set(el); el_set(el, EL_REFRESH); goto again; } - if (!tried && read__fixio(el->el_infd, errno) == 0) + if (!tried && read__fixio(el->el_infd, e) == 0) tried = 1; else { *cp = '\0'; + errno = e; return (-1); } } @@ -369,8 +374,9 @@ el_getc(EditLine *el, char *cp) (void) fprintf(el->el_errfile, "Reading a character\n"); #endif /* DEBUG_READ */ num_read = (*el->el_read.read_char)(el, cp); + el->el_errno = (num_read < 0) ? errno : 0; #ifdef DEBUG_READ - (void) fprintf(el->el_errfile, "Got it %c\n", *cp); + (void) fprintf(el->el_errfile, "Got <%c> (return %d)\n", *cp, num_read); #endif /* DEBUG_READ */ return (num_read); } @@ -426,7 +432,7 @@ el_gets(EditLine *el, int *nread) char *cp = el->el_line.buffer; size_t idx; - while ((*el->el_read.read_char)(el, cp) == 1) { + while ((num = (*el->el_read.read_char)(el, cp)) == 1) { /* make sure there is space for next character */ if (cp + 1 >= el->el_line.limit) { idx = (cp - el->el_line.buffer); @@ -479,7 +485,7 @@ el_gets(EditLine *el, int *nread) term__flush(el); - while ((*el->el_read.read_char)(el, cp) == 1) { + while ((num = (*el->el_read.read_char)(el, cp)) == 1) { /* make sure there is space next character */ if (cp + 1 >= el->el_line.limit) { idx = (cp - el->el_line.buffer); @@ -504,13 +510,14 @@ el_gets(EditLine *el, int *nread) goto done; } - for (num = OKCMD; num == OKCMD;) { /* while still editing this - * line */ + /* While still editing this line */ + for (num = 0;; num = 0) { + enum rcmd rcmd; #ifdef DEBUG_EDIT read_debug(el); #endif /* DEBUG_EDIT */ - /* if EOF or error */ - if ((num = read_getcmd(el, &cmdnum, &ch)) != OKCMD) { + if ((rcmd = read_getcmd(el, &cmdnum, &ch)) != OKCMD) { + num = (rcmd == ERRCMD) ? -1 : 0; #ifdef DEBUG_READ (void) fprintf(el->el_errfile, "Returning from el_gets %d\n", num); @@ -589,9 +596,10 @@ el_gets(EditLine *el, int *nread) continue; /* keep going... */ case CC_EOF: /* end of file typed */ + rcmd = EOFCMD; if ((el->el_flags & UNBUFFERED) == 0) num = 0; - else if (num == -1) { + else { *el->el_line.lastchar++ = CONTROL('d'); el->el_line.cursor = el->el_line.lastchar; num = 1; @@ -599,6 +607,7 @@ el_gets(EditLine *el, int *nread) break; case CC_NEWLINE: /* normal end of line */ + rcmd = EOFCMD; num = (int)(el->el_line.lastchar - el->el_line.buffer); break; @@ -628,6 +637,8 @@ el_gets(EditLine *el, int *nread) el->el_chared.c_vcmd.action = NOP; if (el->el_flags & UNBUFFERED) break; + if (rcmd != OKCMD) + break; } term__flush(el); /* flush any buffered output */ -- 1.7.9.rc2.1.g69204 --=_504f6267.iUCe6UiHu0SzYFl1a67qpbTPwc33pf/H4idnoT7GkXfIKRja Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0002-Add-an-EL_RESTART_READ-option-to-editline-3.patch" >From eb154cc49285ead7852e4e50358d48bb90cda9df Mon Sep 17 00:00:00 2001 Message-Id: <eb154cc49285ead7852e4e50358d48bb90cda9df.1347378617.git.sdaoden@gmail.com> In-Reply-To: <e889bca8dc1fe7ddb95651ecbe98ad05bd59e22e.1347378617.git.sdaoden@gmail.com> References: <e889bca8dc1fe7ddb95651ecbe98ad05bd59e22e.1347378617.git.sdaoden@gmail.com> From: "Steffen \"Daode\" Nurpmeso" <sdaoden@gmail.com> Date: Tue, 11 Sep 2012 15:39:47 +0200 Subject: [PATCH 2/3] Add an EL_RESTART_READ option to editline(3) Make it possible to realize read(2) restarts after EINTR errors without actually going the expensive (and sometimes impossible or at least undesirable) way through signal handling. --- lib/libedit/editline.3 | 20 ++++++++++++++++++++ lib/libedit/el.c | 12 ++++++++++++ lib/libedit/el.h | 1 + lib/libedit/histedit.h | 1 + lib/libedit/read.c | 2 ++ 5 files changed, 36 insertions(+), 0 deletions(-) diff --git a/lib/libedit/editline.3 b/lib/libedit/editline.3 index fe58321..6ecbac8 100644 --- a/lib/libedit/editline.3 +++ b/lib/libedit/editline.3 @@ -385,6 +385,22 @@ check this (using .Fn el_get ) to determine if editing should be enabled or not. +.It Dv EL_RESTART_READ , Fa "int flag" +If +.Fa flag +is not zero (as per default), +then +.Fn el_getc +and +.Fn el_gets +will restart character reads that failed with +.Dv EINTR +errors. +Note this may be restricted to the builtin character read function +.Dv EL_BUILTIN_GETCFN +(see +.Dv EL_GETCFN +below). .It Dv EL_GETCFN , Fa "int (*f)(EditLine *, char *c)" Define the character reading function as .Fa f , @@ -486,6 +502,10 @@ Retrieve previously registered with the corresponding .Fn el_set call. +.It Dv EL_RESTART_READ , Fa "int" +Return non-zero if reading of characters is automatically restarted for +.Dv EINTR +errors. .It Dv EL_UNBUFFERED , Fa "int" Sets or clears unbuffered mode. In this mode, diff --git a/lib/libedit/el.c b/lib/libedit/el.c index d6cfb2d..8418f46 100644 --- a/lib/libedit/el.c +++ b/lib/libedit/el.c @@ -274,6 +274,13 @@ el_set(EditLine *el, int op, ...) el->el_data = va_arg(ap, void *); break; + case EL_RESTART_READ: + if (va_arg(ap, int)) + el->el_flags |= RESTART_READ; + else + el->el_flags &= ~RESTART_READ; + break; + case EL_UNBUFFERED: rv = va_arg(ap, int); if (rv && !(el->el_flags & UNBUFFERED)) { @@ -435,6 +442,11 @@ el_get(EditLine *el, int op, ...) rv = 0; break; + case EL_RESTART_READ: + *va_arg(ap, int *) = ((el->el_flags & RESTART_READ) != 0); + rv = 0; + break; + case EL_UNBUFFERED: *va_arg(ap, int *) = (!(el->el_flags & UNBUFFERED)); rv = 0; diff --git a/lib/libedit/el.h b/lib/libedit/el.h index 67d01ff..e296ff6 100644 --- a/lib/libedit/el.h +++ b/lib/libedit/el.h @@ -55,6 +55,7 @@ #define NO_TTY 0x02 #define EDIT_DISABLED 0x04 #define UNBUFFERED 0x08 +#define RESTART_READ 0x100 typedef int bool_t; /* True or not */ diff --git a/lib/libedit/histedit.h b/lib/libedit/histedit.h index 8a6caf9..5f457f8 100644 --- a/lib/libedit/histedit.h +++ b/lib/libedit/histedit.h @@ -139,6 +139,7 @@ unsigned char _el_fn_sh_complete(EditLine *, int); #define EL_PROMPT_ESC 21 /* , prompt_func, Char); set/get */ #define EL_RPROMPT_ESC 22 /* , prompt_func, Char); set/get */ #define EL_RESIZE 23 /* , el_zfunc_t, void *); set */ +#define EL_RESTART_READ 24 /* , int); set/get */ #define EL_BUILTIN_GETCFN (NULL) diff --git a/lib/libedit/read.c b/lib/libedit/read.c index 0880b5c..4c5996c 100644 --- a/lib/libedit/read.c +++ b/lib/libedit/read.c @@ -304,6 +304,8 @@ read_char(EditLine *el, char *cp) el_set(el, EL_REFRESH); goto again; } + if (e == EINTR && (el->el_flags & RESTART_READ)) + goto again; if (!tried && read__fixio(el->el_infd, e) == 0) tried = 1; else { -- 1.7.9.rc2.1.g69204 --=_504f6267.iUCe6UiHu0SzYFl1a67qpbTPwc33pf/H4idnoT7GkXfIKRja Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0003-Set-EL_RESTART_READ-in-interactive-sh-1-sessions.patch" >From d9ba7f771ff084df56ee8ebacd0d7cb455189ecc Mon Sep 17 00:00:00 2001 Message-Id: <d9ba7f771ff084df56ee8ebacd0d7cb455189ecc.1347378617.git.sdaoden@gmail.com> In-Reply-To: <e889bca8dc1fe7ddb95651ecbe98ad05bd59e22e.1347378617.git.sdaoden@gmail.com> References: <e889bca8dc1fe7ddb95651ecbe98ad05bd59e22e.1347378617.git.sdaoden@gmail.com> From: "Steffen \"Daode\" Nurpmeso" <sdaoden@gmail.com> Date: Tue, 11 Sep 2012 17:50:11 +0200 Subject: [PATCH 3/3] Set EL_RESTART_READ in interactive sh(1) sessions --- bin/sh/histedit.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/bin/sh/histedit.c b/bin/sh/histedit.c index 6371599..67056d0 100644 --- a/bin/sh/histedit.c +++ b/bin/sh/histedit.c @@ -125,6 +125,7 @@ histedit(void) el_set(el, EL_ADDFN, "sh-complete", "Filename completion", _el_fn_sh_complete); + el_set(el, EL_RESTART_READ, 1); } else { bad: out2fmt_flush("sh: can't initialize editing\n"); -- 1.7.9.rc2.1.g69204 --=_504f6267.iUCe6UiHu0SzYFl1a67qpbTPwc33pf/H4idnoT7GkXfIKRja--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?504f6267.yLNFTfIZigyaYVMofL5rUv8U>