bin/169773: sh(1): Resizing causes /bin/sh to repeat edit
operations
Steffen Daode Nurpmeso
sdaoden at gmail.com
Tue Sep 11 16:10:36 UTC 2012
|Mark Johnston <markjdb at 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
-------------- next part --------------
>From e889bca8dc1fe7ddb95651ecbe98ad05bd59e22e Mon Sep 17 00:00:00 2001
Message-Id: <e889bca8dc1fe7ddb95651ecbe98ad05bd59e22e.1347378617.git.sdaoden at gmail.com>
From: "Steffen \"Daode\" Nurpmeso" <sdaoden at 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
-------------- next part --------------
>From eb154cc49285ead7852e4e50358d48bb90cda9df Mon Sep 17 00:00:00 2001
Message-Id: <eb154cc49285ead7852e4e50358d48bb90cda9df.1347378617.git.sdaoden at gmail.com>
In-Reply-To: <e889bca8dc1fe7ddb95651ecbe98ad05bd59e22e.1347378617.git.sdaoden at gmail.com>
References: <e889bca8dc1fe7ddb95651ecbe98ad05bd59e22e.1347378617.git.sdaoden at gmail.com>
From: "Steffen \"Daode\" Nurpmeso" <sdaoden at 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
-------------- next part --------------
>From d9ba7f771ff084df56ee8ebacd0d7cb455189ecc Mon Sep 17 00:00:00 2001
Message-Id: <d9ba7f771ff084df56ee8ebacd0d7cb455189ecc.1347378617.git.sdaoden at gmail.com>
In-Reply-To: <e889bca8dc1fe7ddb95651ecbe98ad05bd59e22e.1347378617.git.sdaoden at gmail.com>
References: <e889bca8dc1fe7ddb95651ecbe98ad05bd59e22e.1347378617.git.sdaoden at gmail.com>
From: "Steffen \"Daode\" Nurpmeso" <sdaoden at 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
More information about the freebsd-bugs
mailing list