Source-Changes-HG archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
[src/trunk]: src/lib/libedit From Ingo Schwarze:
details: https://anonhg.NetBSD.org/src/rev/3509d8b95b1d
branches: trunk
changeset: 815536:3509d8b95b1d
user: christos <christos%NetBSD.org@localhost>
date: Tue May 24 15:00:45 2016 +0000
description:
>From Ingo Schwarze:
Reduce obfuscation of errno handling. There is only one purpose
non-local errno handling is needed for: Inside el_wgets(), several
functions call down indirectly to el_wgetc(), many of them via the
dispatch table. When el_wgetc() fails, it does properly report
failure, but then various cleanup is done which may clobber errno.
But when returning due to failure, el_wgets() wants to have errno
set to the reason of the original read failure, not to the reason
of some subsequent failure of some cleanup operation. So el_wgetc()
needs to save errno, and if it's non-zero, el_wgets() needs to
restore it on failure.
This core logic is currently obscured by the fact that el_errno
is set and inspected at some additional places where it isn't needed.
Besides, since el_wgetc() and and el_wgets() are both in read.c,
el_errno does not need to be in struct editline, it can and should
be local to read.c in struct el_read_t.
Let's look at what can be simplified.
1. keymacro_get() abuses el_errno instead of having a proper
error return code. Adding that error return code is easy
because node_trav() already detects the condition and an
adequate code is already defined. Returning it, testing
for it in read_getcmd(), and returning with error from there
removes the need to inspect el_errno from el_wgets() after
calling read_getcmd().
Note that resetting lastchar and cursor and clearing buffer[0]
is irrelevant. The code returns from el_wgets() right afterwards.
Outside el_wgets(), these variables are no longer relevant.
When el_wgets() is called the next time, it will call ch_reset()
anyway, resetting the two pointers. And as long as lastchar
points to the beginning of the buffer, the contents of the
buffer won't be used for anything.
2. read_getcmd() doesn't need to set el_errno again after el_wgetc()
failure since el_wgetc() already did so. While here, remove
the silly "if EOF or error" comments from the el_wgetc()
return value tests. It's a public interface documented in a
manual, so people working on the implementation can obviously
be expected to know how it works. It's a case of
count++; /* Increment count. */
3. In the two code paths of el_wgets() that lead up to "goto noedit",
there is no need to save the errno because nothing that might
change it happens before returning.
For clarity, since el_wgets() is the function restoring the errno,
also move initializing it to the same function.
Finally, note that restoring errno when the saved value is zero is
wrong. No library code is ever allowed to clear a previously set
value of errno. Only application programs are allowed to do that,
and even they usually don't need to do so, except when using certain
ill-designed interfaces like strtol(3).
I tested that the behaviour remains sane in the following cases,
all during execution of el_wgets(3) and with a signal handler
for USR1 installed without SA_RESTART.
* Enter some text and maybe move around a bit.
Then send a USR1 signal.
The signal gets processed, then read_char() resumes reading.
Send another USR1 signal.
Now el_wgets() sets errno=EINTR and returns -1.
* Press Ctrl-V to activate ed-quoted-insert.
Then send a USR1 signal.
The signal gets processed, then read_char() resumes reading.
Send another USR1 signal.
ed_quoted_insert() returns ed_end_of_file(), i.e. CC_EOF,
and el_wgets() returns 0.
* Press a key starting a keyboard macro.
Then send a USR1 signal.
The signal gets processed, then read_char() resumes reading.
Send another USR1 signal.
Now el_wgets() sets errno=EINTR and returns -1.
* Press : to enter builtin command mode.
Start typing a command.
Then send a USR1 signal.
The signal gets processed, then read_char() resumes reading.
Send another USR1 signal.
Now c_gets() returns -1, ed_command() beeps and returns CC_REFRESH,
and el_wgets() resumes operation as it should.
I also tested with "el_set(el, EL_EDITMODE, 0)", and it returns
the right value and sets errno correctly.
diffstat:
lib/libedit/el.h | 3 +--
lib/libedit/keymacro.c | 12 +++++-------
lib/libedit/read.c | 48 +++++++++++++++++++++---------------------------
3 files changed, 27 insertions(+), 36 deletions(-)
diffs (185 lines):
diff -r 63d35be0efd9 -r 3509d8b95b1d lib/libedit/el.h
--- a/lib/libedit/el.h Tue May 24 13:57:26 2016 +0000
+++ b/lib/libedit/el.h Tue May 24 15:00:45 2016 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: el.h,v 1.40 2016/05/09 21:46:56 christos Exp $ */
+/* $NetBSD: el.h,v 1.41 2016/05/24 15:00:45 christos Exp $ */
/*-
* Copyright (c) 1992, 1993
@@ -114,7 +114,6 @@
int el_outfd; /* Output file descriptor */
int el_errfd; /* Error file descriptor */
int el_flags; /* Various flags. */
- int el_errno; /* Local copy of errno */
coord_t el_cursor; /* Cursor location */
wchar_t **el_display; /* Real screen image = what is there */
wchar_t **el_vdisplay; /* Virtual screen image = what we see */
diff -r 63d35be0efd9 -r 3509d8b95b1d lib/libedit/keymacro.c
--- a/lib/libedit/keymacro.c Tue May 24 13:57:26 2016 +0000
+++ b/lib/libedit/keymacro.c Tue May 24 15:00:45 2016 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: keymacro.c,v 1.22 2016/05/09 21:46:56 christos Exp $ */
+/* $NetBSD: keymacro.c,v 1.23 2016/05/24 15:00:45 christos Exp $ */
/*-
* Copyright (c) 1992, 1993
@@ -37,7 +37,7 @@
#if 0
static char sccsid[] = "@(#)key.c 8.1 (Berkeley) 6/4/93";
#else
-__RCSID("$NetBSD: keymacro.c,v 1.22 2016/05/09 21:46:56 christos Exp $");
+__RCSID("$NetBSD: keymacro.c,v 1.23 2016/05/24 15:00:45 christos Exp $");
#endif
#endif /* not lint && not SCCSID */
@@ -172,6 +172,7 @@
* complete match is found or a mismatch occurs. Returns the
* type of the match found (XK_STR or XK_CMD).
* Returns NULL in val.str and XK_STR for no match.
+ * Returns XK_NOD for end of file or read error.
* The last character read is returned in *ch.
*/
libedit_private int
@@ -286,11 +287,8 @@
/* match found */
if (ptr->next) {
/* key not complete so get next char */
- if (el_wgetc(el, ch) != 1) {/* if EOF or error */
- val->cmd = ED_END_OF_FILE;
- return XK_CMD;
- /* PWP: Pretend we just read an end-of-file */
- }
+ if (el_wgetc(el, ch) != 1)
+ return XK_NOD;
return node_trav(el, ptr->next, ch, val);
} else {
*val = ptr->val;
diff -r 63d35be0efd9 -r 3509d8b95b1d lib/libedit/read.c
--- a/lib/libedit/read.c Tue May 24 13:57:26 2016 +0000
+++ b/lib/libedit/read.c Tue May 24 15:00:45 2016 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: read.c,v 1.97 2016/05/22 19:44:26 christos Exp $ */
+/* $NetBSD: read.c,v 1.98 2016/05/24 15:00:45 christos Exp $ */
/*-
* Copyright (c) 1992, 1993
@@ -37,7 +37,7 @@
#if 0
static char sccsid[] = "@(#)read.c 8.1 (Berkeley) 6/4/93";
#else
-__RCSID("$NetBSD: read.c,v 1.97 2016/05/22 19:44:26 christos Exp $");
+__RCSID("$NetBSD: read.c,v 1.98 2016/05/24 15:00:45 christos Exp $");
#endif
#endif /* not lint && not SCCSID */
@@ -68,6 +68,7 @@
struct el_read_t {
struct macros macros;
el_rfunc_t read_char; /* Function to read a character. */
+ int read_errno;
};
static int read__fixio(int, int);
@@ -248,12 +249,9 @@
el_action_t cmd;
int num;
- el->el_errno = 0;
do {
- if ((num = el_wgetc(el, ch)) != 1) {/* if EOF or error */
- el->el_errno = num == 0 ? 0 : errno;
+ if ((num = el_wgetc(el, ch)) != 1)
return -1;
- }
#ifdef KANJI
if ((*ch & meta)) {
@@ -280,6 +278,8 @@
case XK_STR:
el_wpush(el, val.str);
break;
+ case XK_NOD:
+ return -1;
default:
EL_ABORT((el->el_errfile, "Bad XK_ type \n"));
break;
@@ -438,8 +438,15 @@
(void) fprintf(el->el_errfile, "Reading a character\n");
#endif /* DEBUG_READ */
num_read = (*el->el_read->read_char)(el, cp);
+
+ /*
+ * Remember the original reason of a read failure
+ * such that el_wgets() can restore it after doing
+ * various cleanup operation that might change errno.
+ */
if (num_read < 0)
- el->el_errno = errno;
+ el->el_read->read_errno = errno;
+
#ifdef DEBUG_READ
(void) fprintf(el->el_errfile, "Got it %lc\n", *cp);
#endif /* DEBUG_READ */
@@ -490,6 +497,7 @@
if (nread == NULL)
nread = &nrb;
*nread = 0;
+ el->el_read->read_errno = 0;
if (el->el_flags & NO_TTY) {
size_t idx;
@@ -510,12 +518,8 @@
if (cp[-1] == '\r' || cp[-1] == '\n')
break;
}
- if (num == -1) {
- if (errno == EINTR)
- cp = el->el_line.buffer;
- el->el_errno = errno;
- }
-
+ if (num == -1 && errno == EINTR)
+ cp = el->el_line.buffer;
goto noedit;
}
@@ -564,13 +568,8 @@
if (crlf)
break;
}
-
- if (num == -1) {
- if (errno == EINTR)
- cp = el->el_line.buffer;
- el->el_errno = errno;
- }
-
+ if (num == -1 && errno == EINTR)
+ cp = el->el_line.buffer;
goto noedit;
}
@@ -586,12 +585,6 @@
#endif /* DEBUG_READ */
break;
}
- if (el->el_errno == EINTR) {
- el->el_line.buffer[0] = '\0';
- el->el_line.lastchar =
- el->el_line.cursor = el->el_line.buffer;
- break;
- }
if ((size_t)cmdnum >= el->el_map.nfunc) { /* BUG CHECK command */
#ifdef DEBUG_EDIT
(void) fprintf(el->el_errfile,
@@ -723,7 +716,8 @@
if (*nread == 0) {
if (num == -1) {
*nread = -1;
- errno = el->el_errno;
+ if (el->el_read->read_errno)
+ errno = el->el_read->read_errno;
}
return NULL;
} else
Home |
Main Index |
Thread Index |
Old Index