Mark Johnston <mark...@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
seems to be the easiest approach to get around this.
Other options would have been to implement an el_gets_continue(),
which would have restarted editing with the input of the last
state (but what if that ended in a newline?), or to commit suicide
while trying to deal with signals from within sh(1).
I have also tried to extend editline.3 in respect to
EL_UNBUFFERED, which is only partially documented sofar, and the
yet completely undocumented errno handling.
It is a whole series of local commits indeed, but i don't dare to
attach a MBOX or even (horror) send a patch mail-series, and so
i'll simply attach them in order, including the PR bin/170651
patch (laziness). It seems to work. In reversed order:
- 6.diff: Set EL_READRESTART in interactive sh(1) sessions
- 5.diff: Add a new EL_READRESTART option for editline(3)
- 4.diff: Document errno behaviour of el_getc()/el_gets()
- 3.diff: Document EL_UNBUFFERED for el_set()
- 2.diff: Fix editline(3) char read and errno code flow
: This simply reuses your patch.
- 1.diff: Fix PR bin/170651
: (The plain patch. Maybe possible to leave that off.)
|-Mark
Bye and ciao,
--steffen
commit b68195b7d21912bd13b74412db43e2dbecdd4b92
Author: Steffen Daode Nurpmeso <sdao...@gmail.com>
Date: 2012-09-01 17:21:14 +0200
Fix PR bin/170651
diff --git a/bin/sh/histedit.c b/bin/sh/histedit.c
index 6371599..bd47c0d 100644
--- a/bin/sh/histedit.c
+++ b/bin/sh/histedit.c
@@ -67,7 +67,9 @@ __FBSDID("$FreeBSD$");
History *hist; /* history cookie */
EditLine *el; /* editline cookie */
int displayhist;
+int histedit_init;
static FILE *el_in, *el_out, *el_err;
+static int e1v2;
static char *fc_replace(const char *, char *, char *);
static int not_fcnumber(const char *);
@@ -76,12 +78,21 @@ static int str_to_event(const char *, int);
/*
* Set history and editing status. Called whenever the status may
* have changed (figures out what to do).
+ * If force is set then an editline reinit is issued even if the actual edit
+ * mode hasn't changed - necessary after the locale has changed because
+ * editline bases it's decision what is reported or not upon isprint(3)
*/
void
-histedit(void)
+histedit(int force)
{
+ int nedstate;
-#define editing (Eflag || Vflag)
+ if (! histedit_init)
+ return;
+
+ histedit_init = 2;
+ /* options.c ensures these are mutual exclusive */
+ nedstate = (Eflag ? 1 : 0) | (Vflag ? 2 : 0);
if (iflag) {
if (!hist) {
@@ -97,7 +108,7 @@ histedit(void)
else
out2fmt_flush("sh: can't initialize history\n");
}
- if (editing && !el && isatty(0)) { /* && isatty(2) ??? */
+ if (nedstate && ! el && isatty(0)) { /* && isatty(2) ??? */
/*
* turn editing on
*/
@@ -130,17 +141,14 @@ bad:
out2fmt_flush("sh: can't initialize editing\n");
}
INTON;
- } else if (!editing && el) {
+ } else if (! nedstate && el) {
INTOFF;
el_end(el);
el = NULL;
INTON;
}
- if (el) {
- if (Vflag)
- el_set(el, EL_EDITOR, "vi");
- else if (Eflag)
- el_set(el, EL_EDITOR, "emacs");
+ if (el && (nedstate != e1v2 || force)) {
+ el_set(el, EL_EDITOR, (nedstate & 1) ? "emacs" : "vi");
el_set(el, EL_BIND, "^I", "sh-complete", NULL);
el_source(el, NULL);
}
@@ -155,7 +163,10 @@ bad:
hist = NULL;
}
INTON;
+ nedstate = 0;
}
+
+ e1v2 = nedstate;
}
diff --git a/bin/sh/input.c b/bin/sh/input.c
index 12f285f..1da03c6 100644
--- a/bin/sh/input.c
+++ b/bin/sh/input.c
@@ -59,8 +59,10 @@ __FBSDID("$FreeBSD$");
#include "error.h"
#include "alias.h"
#include "parser.h"
-#include "myhistedit.h"
#include "trap.h"
+#ifndef NO_HISTORY
+# include "myhistedit.h"
+#endif
#define EOF_NLEFT -99 /* value of parsenleft when EOF pushed back */
@@ -102,8 +104,6 @@ static struct parsefile *parsefile = &basepf; /*
current input file */
int init_editline = 0; /* editline library initialized? */
int whichprompt; /* 1 == PS1, 2 == PS2 */
-EditLine *el; /* cookie for editline package */
-
static void pushfile(void);
static int preadfd(void);
static void popstring(void);
diff --git a/bin/sh/main.c b/bin/sh/main.c
index 5eb12e0..684ce8c 100644
--- a/bin/sh/main.c
+++ b/bin/sh/main.c
@@ -73,6 +73,9 @@ __FBSDID("$FreeBSD$");
#include "exec.h"
#include "cd.h"
#include "builtins.h"
+#ifndef NO_HISTORY
+# include "myhistedit.h"
+#endif
int rootpid;
int rootshell;
@@ -144,8 +147,12 @@ main(int argc, char *argv[])
setstackmark(&smark2);
procargs(argc, argv);
pwd_init(iflag);
- if (iflag)
+ if (iflag) {
chkmail(1);
+#ifndef NO_HISTORY
+ histedit_init = 1;
+#endif
+ }
if (argv[0] && argv[0][0] == '-') {
state = 1;
read_profile("/etc/profile");
@@ -157,6 +164,10 @@ state1:
read_profile("/etc/suid_profile");
}
state2:
+#ifndef NO_HISTORY
+ if (iflag && histedit_init != 2)
+ histedit(1);
+#endif
state = 3;
if (!privileged && iflag) {
if ((shinit = lookupvar("ENV")) != NULL && *shinit != '\0') {
diff --git a/bin/sh/myhistedit.h b/bin/sh/myhistedit.h
index e31276d..24123e1 100644
--- a/bin/sh/myhistedit.h
+++ b/bin/sh/myhistedit.h
@@ -35,8 +35,8 @@
extern History *hist;
extern EditLine *el;
extern int displayhist;
+extern int histedit_init;
-void histedit(void);
+void histedit(int);
void sethistsize(const char *);
void setterm(const char *);
-
diff --git a/bin/sh/options.c b/bin/sh/options.c
index ad0291e..f84b4d8 100644
--- a/bin/sh/options.c
+++ b/bin/sh/options.c
@@ -58,7 +58,7 @@ __FBSDID("$FreeBSD$");
#include "mystring.h"
#include "builtins.h"
#ifndef NO_HISTORY
-#include "myhistedit.h"
+# include "myhistedit.h"
#endif
char *arg0; /* value of $0 */
@@ -131,7 +131,7 @@ optschanged(void)
{
setinteractive(iflag);
#ifndef NO_HISTORY
- histedit();
+ histedit(0);
#endif
setjobctl(mflag);
}
diff --git a/bin/sh/trap.c b/bin/sh/trap.c
index 521c511..b0d8b11 100644
--- a/bin/sh/trap.c
+++ b/bin/sh/trap.c
@@ -56,8 +56,9 @@ __FBSDID("$FreeBSD$");
#include "trap.h"
#include "mystring.h"
#include "builtins.h"
-#include "myhistedit.h"
-
+#ifndef NO_HISTORY
+# include "myhistedit.h"
+#endif
/*
* Sigmode records the current value of the signal handlers for the various
diff --git a/bin/sh/var.c b/bin/sh/var.c
index 6041459..d1af678 100644
--- a/bin/sh/var.c
+++ b/bin/sh/var.c
@@ -65,7 +65,7 @@ __FBSDID("$FreeBSD$");
#include "parser.h"
#include "builtins.h"
#ifndef NO_HISTORY
-#include "myhistedit.h"
+# include "myhistedit.h"
#endif
@@ -523,6 +523,9 @@ updatecharset(void)
charset = nl_langinfo(CODESET);
localeisutf8 = !strcmp(charset, "UTF-8");
+#ifndef NO_HISTORY
+ histedit(1);
+#endif
}
void
commit 8582cb9cab300fe486cfeeb46142e3cfd9cfe962
Author: Steffen Daode Nurpmeso <sdao...@gmail.com>
Date: 2012-09-10 15:50:58 +0200
Fix editline(3) char read and errno code flow
The reading call chain failed to initialize local variables
and also did not map return values that it got back from
deeper in the call chain to its own meaning of those states,
thus failing later due to misunderstanding.
In addition the tracking of errno in EditLine::el_errno, and
vice versa was also broken and is now fixed.
diff --git a/lib/libedit/read.c b/lib/libedit/read.c
index 7d7f54b..ecd1ee2 100644
--- a/lib/libedit/read.c
+++ b/lib/libedit/read.c
@@ -238,8 +238,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 ? 1 : 0);
}
#ifdef KANJI
@@ -294,16 +293,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 +370,10 @@ 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);
+ if (num_read < 0)
+ el->el_errno = errno;
#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 +429,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 +482,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);
@@ -511,6 +514,7 @@ el_gets(EditLine *el, int *nread)
#endif /* DEBUG_EDIT */
/* if EOF or error */
if ((num = read_getcmd(el, &cmdnum, &ch)) != OKCMD) {
+ num = -1;
#ifdef DEBUG_READ
(void) fprintf(el->el_errfile,
"Returning from el_gets %d\n", num);
commit 8bc3e8e2d6b42b422ef87e5da916b162cf2fd7d5
Author: Steffen Daode Nurpmeso <sdao...@gmail.com>
Date: 2012-09-10 15:23:37 +0200
Document EL_UNBUFFERED for el_set()
diff --git a/lib/libedit/editline.3 b/lib/libedit/editline.3
index fe58321..a08a0f0 100644
--- a/lib/libedit/editline.3
+++ b/lib/libedit/editline.3
@@ -385,6 +385,14 @@ check this
(using
.Fn el_get )
to determine if editing should be enabled or not.
+.It Dv EL_UNBUFFERED , Fa "int flag"
+If
+.Fa flag
+is zero,
+unbuffered mode is disabled (the default).
+In unbuffered mode,
+.Fn el_gets
+will return immediately after processing a single character.
.It Dv EL_GETCFN , Fa "int (*f)(EditLine *, char *c)"
Define the character reading function as
.Fa f ,
@@ -487,10 +495,7 @@ previously registered with the corresponding
.Fn el_set
call.
.It Dv EL_UNBUFFERED , Fa "int"
-Sets or clears unbuffered mode.
-In this mode,
-.Fn el_gets
-will return immediately after processing a single character.
+Return non-zero if unbuffered mode is enabled.
.It Dv EL_PREP_TERM , Fa "int"
Sets or clears terminal editing mode.
.It Dv EL_GETFP , Fa "int fd", Fa "FILE **fp"
commit 2b9d0ef804cbb44d9caf580ee53b1241b6904aa0
Author: Steffen Daode Nurpmeso <sdao...@gmail.com>
Date: 2012-09-10 15:24:29 +0200
Document errno behaviour of el_getc()/el_gets()
diff --git a/lib/libedit/editline.3 b/lib/libedit/editline.3
index a08a0f0..1f26b39 100644
--- a/lib/libedit/editline.3
+++ b/lib/libedit/editline.3
@@ -174,7 +174,10 @@ and must be copied if the data is to be retained.
Read a character from the tty.
.Fa ch
is modified to contain the character read.
-Returns the number of characters read if successful, \-1 otherwise.
+Returns the number of characters read if successful, \-1 otherwise,
+in which case
+.Dv errno
+is set.
.It Fn el_push
Pushes
.Fa str
@@ -397,7 +400,8 @@ will return immediately after processing a single character.
Define the character reading function as
.Fa f ,
which is to return the number of characters read and store them in
-.Fa c .
+.Fa c ,
+and -1 on failure with errno set to the failure.
This function is called internally by
.Fn el_gets
and
@@ -788,6 +792,7 @@ is a NUL terminated string to tokenize.
.\"XXX: provide some examples
.Sh SEE ALSO
.Xr sh 1 ,
+.Xr intro 2 ,
.Xr signal 3 ,
.Xr termcap 3 ,
.Xr editrc 5 ,
commit a548d383b230b02d1ca0fc44acc384a32d82abac
Author: Steffen Daode Nurpmeso <sdao...@gmail.com>
Date: 2012-09-10 15:39:00 +0200
Add a new EL_READRESTART option for editline(3)
Which makes it possible to realize read(2) restarts after EINTR
errors without actually going the expensive (and sometimes
impossible) way through signal handling.
Unfortunately editline(3) doesn't offer anything like
"el_gets_continue()", which could be used to simply restart the
last editing session at the point where it ended, and it seems
much harder to add that functionality (also in respect to merging
in between *BSD versions) than to add this flag.
diff --git a/lib/libedit/editline.3 b/lib/libedit/editline.3
index 1f26b39..7bb783e 100644
--- a/lib/libedit/editline.3
+++ b/lib/libedit/editline.3
@@ -388,6 +388,22 @@ check this
(using
.Fn el_get )
to determine if editing should be enabled or not.
+.It Dv EL_READRESTART , Fa "int flag"
+If
+.Fa flag
+is zero (the default),
+then
+.Fn el_getc
+and
+.Fn el_gets
+will not treat
+.Dv EINTR
+errors any special and automatically restart reading characters.
+Note this may be restricted to the builtin character read function
+.Dv EL_BUILTIN_GETCFN
+(see
+.Dv EL_GETCFN
+below).
.It Dv EL_UNBUFFERED , Fa "int flag"
If
.Fa flag
@@ -498,6 +514,9 @@ Retrieve
previously registered with the corresponding
.Fn el_set
call.
+.It Dv EL_READRESTART , Fa "int"
+Return non-zero if reading of characters is restarted after signal
+interruption.
.It Dv EL_UNBUFFERED , Fa "int"
Return non-zero if unbuffered mode is enabled.
.It Dv EL_PREP_TERM , Fa "int"
diff --git a/lib/libedit/el.c b/lib/libedit/el.c
index d6cfb2d..4be765f 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_READRESTART:
+ if (va_arg(ap, int))
+ el->el_flags |= READRESTART;
+ else
+ el->el_flags &= ~READRESTART;
+ 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_READRESTART:
+ *va_arg(ap, int *) = (el->el_flags & READRESTART) != 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..d1321cc 100644
--- a/lib/libedit/el.h
+++ b/lib/libedit/el.h
@@ -54,7 +54,8 @@
#define HANDLE_SIGNALS 0x01
#define NO_TTY 0x02
#define EDIT_DISABLED 0x04
-#define UNBUFFERED 0x08
+#define READRESTART 0x08
+#define UNBUFFERED 0x10
typedef int bool_t; /* True or not */
diff --git a/lib/libedit/histedit.h b/lib/libedit/histedit.h
index 8a6caf9..13d0cbf 100644
--- a/lib/libedit/histedit.h
+++ b/lib/libedit/histedit.h
@@ -130,15 +130,16 @@ unsigned char _el_fn_sh_complete(EditLine *, int);
#define EL_RPROMPT 12 /* , el_pfunc_t); */
#define EL_GETCFN 13 /* , el_rfunc_t); */
#define EL_CLIENTDATA 14 /* , void *); */
-#define EL_UNBUFFERED 15 /* , int); */
-#define EL_PREP_TERM 16 /* , int); */
-#define EL_GETTC 17 /* , const char *, ..., NULL); */
-#define EL_GETFP 18 /* , int, FILE **); */
-#define EL_SETFP 19 /* , int, FILE *); */
-#define EL_REFRESH 20 /* , void);
set */
-#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_READRESTART 15 /* , int); */
+#define EL_UNBUFFERED 16 /* , int); */
+#define EL_PREP_TERM 17 /* , int); */
+#define EL_GETTC 18 /* , const char *, ..., NULL); */
+#define EL_GETFP 19 /* , int, FILE **); */
+#define EL_SETFP 20 /* , int, FILE *); */
+#define EL_REFRESH 21 /* , void);
set */
+#define EL_PROMPT_ESC 22 /* , prompt_func, Char);
set/get */
+#define EL_RPROMPT_ESC 23 /* , prompt_func, Char);
set/get */
+#define EL_RESIZE 24 /* , el_zfunc_t, void *);
set */
#define EL_BUILTIN_GETCFN (NULL)
diff --git a/lib/libedit/read.c b/lib/libedit/read.c
index ecd1ee2..3634d7e 100644
--- a/lib/libedit/read.c
+++ b/lib/libedit/read.c
@@ -300,6 +300,8 @@ read_char(EditLine *el, char *cp)
el_set(el, EL_REFRESH);
goto again;
}
+ if (e == EINTR && (el->el_flags & READRESTART))
+ goto again;
if (!tried && read__fixio(el->el_infd, e) == 0)
tried = 1;
else {
commit 55f0420fc73153132fabadd84edc46e569fc0a50 (HEAD, refs/heads/i)
Author: Steffen Daode Nurpmeso <sdao...@gmail.com>
Date: 2012-09-10 15:14:30 +0200
Set EL_READRESTART in interactive sh(1) sessions
This closes PR bin/169773.
diff --git a/bin/sh/histedit.c b/bin/sh/histedit.c
index bd47c0d..e4ef7e0 100644
--- a/bin/sh/histedit.c
+++ b/bin/sh/histedit.c
@@ -136,6 +136,7 @@ histedit(int force)
el_set(el, EL_ADDFN, "sh-complete",
"Filename completion",
_el_fn_sh_complete);
+ el_set(el, EL_READRESTART, 1);
} else {
bad:
out2fmt_flush("sh: can't initialize editing\n");
_______________________________________________
freebsd-bugs@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-bugs
To unsubscribe, send any mail to "freebsd-bugs-unsubscr...@freebsd.org"
