Date: Sat, 8 Sep 2012 17:29:44 -0400 From: Mark Johnston <markjdb@gmail.com> To: Steffen Daode Nurpmeso <sdaoden@gmail.com>, bug-followup@FreeBSD.org Cc: jilles@FreeBSD.org, freebsd-bugs@FreeBSD.org, eadler@FreeBSD.org Subject: Re: bin/169773: sh(1): Resizing causes /bin/sh to repeat edit operations Message-ID: <20120908212944.GA39382@raichu.mark-home> In-Reply-To: <504b67f8.hU%2BoKTUxDdq8oHD59qxE1DdF@dietcurd.wild-life.local> References: <201209061926.q86JQwAC087821@freefall.freebsd.org> <504b67f8.hU%2BoKTUxDdq8oHD59qxE1DdF@dietcurd.wild-life.local>
next in thread | previous in thread | raw e-mail | index | archive | help
--AqsLC8rIMeq19msA Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > > Oh, what a mess :) > I agree with Mark that the handling of OKCMD is simply wrong, but > it turned out to be wrong to simply use a different value for it. > Also the passing through of errno is incomplete in there. (The > documented interface doesn't state anything about errno or useful > error handling at all, and however!) > > It's a rather quick first diff for editline(3), i have no more > time but since it took almost three hours to come that far someone > else may build on top of it (or simply try it or ... wait for a > second). 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 > gets "thrown away". (I guess the supposed way is to temporarily > adjust PROMPT if one wants to continue what is on the line yet? > But i still have no idea of editline(3) :->) > > 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. -Mark --AqsLC8rIMeq19msA Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="libedit_error_handling.patch" 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); --AqsLC8rIMeq19msA--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120908212944.GA39382>