Re: libedit: stop playing hopeless games with FIONBIO
On Wed, 11 Aug 2021 18:50:10 +0200, Ingo Schwarze wrote: > in its current state, the editline(3) library is completely unfit > to work with non-blocking I/O. Neither the API nor the implementation > are even designed for that purpose. > > Consequently, i propose that we document the facts up front and > simply delete some code that isn't going to do any good in practice. > For programs not using non-blocking I/O on the editline input file > descriptor, this patch makes no difference. Programs that do set > FIONBIO on a file descriptor and then call editline(3) on it anyway > can't possibly work as intended as far as i can see. Either setting > FIONBIO is needless in the first place, or el_gets(3) clearing it > will ruin the rest of the program's functionality. Fine with me. - todd
Re: libedit: stop playing hopeless games with FIONBIO
On Wed, 2021-08-11 at 18:50 +0200, Ingo Schwarze wrote: > Hi, > > in its current state, the editline(3) library is completely unfit > to work with non-blocking I/O. Neither the API nor the implementation > are even designed for that purpose. > > I do have some candidate ideas to minimally extend the API - without > adding any new functions - to also work with non-blocking I/O, but > frankly, i don't see any sane use case for it yet. Why would anyone > desire to mix non-blocking I/O and interactive line editing with the > editline(3) library on the same file descriptor? I can't think of anything. If you want to combine with something that also uses other resources you should probably put something like kqueue in front of it anyway. That being said and a completely different issue: if this construct is the case and someone enters a single byte of a UTF-8 character your whole event-based system grinds to a halt until the rest of the character is read. > > Consequently, i propose that we document the facts up front and > simply delete some code that isn't going to do any good in practice. > For programs not using non-blocking I/O on the editline input file > descriptor, this patch makes no difference. Programs that do set > FIONBIO on a file descriptor and then call editline(3) on it anyway > can't possibly work as intended as far as i can see. Either setting > FIONBIO is needless in the first place, or el_gets(3) clearing it > will ruin the rest of the program's functionality. > > Do any of you have any doubts, and if so, which ones? Sounds reasonable, but I'm not aware enough of the libedit consumers to make a definitive statement. Patch reads mostly OK to me. > > Or is anybody willing to OK this patch? > > > If soembody comes up with a sane use case for mixing FIONBIO with > editline(3) at a later point in time, we can still consider my plans > to make that work. In that case, deleting the junk deleted by this > patch would be required as the first step anyway. > > > I'm appending a simple test program at the end. > > Before the patch: > > nbio is on > ?test > the user said: test > nbio is off /* oops, what is this? */ > > After the patch: > > nbio is on > ?progname: el_gets: Resource temporarily unavailable > > I think the latter makes more sense and is less surprising. > > Yours, > Ingo > > > Index: editline.3 > === > RCS file: /cvs/src/lib/libedit/editline.3,v > retrieving revision 1.46 > diff -u -p -r1.46 editline.3 > --- editline.3 22 May 2016 22:08:42 - 1.46 > +++ editline.3 11 Aug 2021 16:20:00 - > @@ -169,6 +169,20 @@ Programs should be linked with > .Pp > The > .Nm > +library is designed to work with blocking I/O only. > +If the > +.Dv FIONBIO > +.Xr ioctl 2 I'm not sure if FIONBIO should be mentioned here. Else you would also have to mention O_NONBLOCK and O_NDELAY. > +is set on > +.Ar fin , > +.Fn el_gets > +and > +.Fn el_wgets > +will almost certainly fail with > +.Ar EAGAIN . > +.Pp > +The > +.Nm > library respects the > .Ev LC_CTYPE > locale set by the application program and never uses > Index: read.c > === > RCS file: /cvs/src/lib/libedit/read.c,v > retrieving revision 1.47 > diff -u -p -r1.47 read.c > --- read.c 11 Aug 2021 15:13:46 - 1.47 > +++ read.c 11 Aug 2021 16:20:00 - > @@ -66,7 +66,6 @@ struct el_read_t { > int read_errno; > }; > > -static int read__fixio(int, int); > static int read_char(EditLine *, wchar_t *); > static int read_getcmd(EditLine *, el_action_t *, wchar_t *); > static voidread_clearmacros(struct macros *); > @@ -132,26 +131,6 @@ el_read_getfn(struct el_read_t *el_read) > } > > > -/* read__fixio(): > - * Try to recover from a read error > - */ > -static int > -read__fixio(int fd, int e) > -{ > - int zero = 0; > - > - switch (e) { > - case EAGAIN: > - if (ioctl(fd, FIONBIO, ) == -1) > - return -1; > - return 0; > - > - default: > - return -1; > - } > -} > - > - > /* el_push(): > * Push a macro > */ > @@ -231,15 +210,12 @@ static int > read_char(EditLine *el, wchar_t *cp) > { > ssize_t num_read; > - int tried = 0; > char cbuf[MB_LEN_MAX]; > int cbp = 0; > - int save_errno = errno; > > again: > el->el_signal->sig_no = 0; > while ((num_read = read(el->el_infd, cbuf + cbp, 1)) == -1) { > - int e = errno; > if (errno == EINTR) { > switch (el->el_signal->sig_no) { > case SIGCONT: > @@ -252,14 +228,8 @@ read_char(EditLine *el, wchar_t *cp) > break; > } > } > - if (!tried &&
libedit: stop playing hopeless games with FIONBIO
Hi, in its current state, the editline(3) library is completely unfit to work with non-blocking I/O. Neither the API nor the implementation are even designed for that purpose. I do have some candidate ideas to minimally extend the API - without adding any new functions - to also work with non-blocking I/O, but frankly, i don't see any sane use case for it yet. Why would anyone desire to mix non-blocking I/O and interactive line editing with the editline(3) library on the same file descriptor? Consequently, i propose that we document the facts up front and simply delete some code that isn't going to do any good in practice. For programs not using non-blocking I/O on the editline input file descriptor, this patch makes no difference. Programs that do set FIONBIO on a file descriptor and then call editline(3) on it anyway can't possibly work as intended as far as i can see. Either setting FIONBIO is needless in the first place, or el_gets(3) clearing it will ruin the rest of the program's functionality. Do any of you have any doubts, and if so, which ones? Or is anybody willing to OK this patch? If soembody comes up with a sane use case for mixing FIONBIO with editline(3) at a later point in time, we can still consider my plans to make that work. In that case, deleting the junk deleted by this patch would be required as the first step anyway. I'm appending a simple test program at the end. Before the patch: nbio is on ?test the user said: test nbio is off/* oops, what is this? */ After the patch: nbio is on ?progname: el_gets: Resource temporarily unavailable I think the latter makes more sense and is less surprising. Yours, Ingo Index: editline.3 === RCS file: /cvs/src/lib/libedit/editline.3,v retrieving revision 1.46 diff -u -p -r1.46 editline.3 --- editline.3 22 May 2016 22:08:42 - 1.46 +++ editline.3 11 Aug 2021 16:20:00 - @@ -169,6 +169,20 @@ Programs should be linked with .Pp The .Nm +library is designed to work with blocking I/O only. +If the +.Dv FIONBIO +.Xr ioctl 2 +is set on +.Ar fin , +.Fn el_gets +and +.Fn el_wgets +will almost certainly fail with +.Ar EAGAIN . +.Pp +The +.Nm library respects the .Ev LC_CTYPE locale set by the application program and never uses Index: read.c === RCS file: /cvs/src/lib/libedit/read.c,v retrieving revision 1.47 diff -u -p -r1.47 read.c --- read.c 11 Aug 2021 15:13:46 - 1.47 +++ read.c 11 Aug 2021 16:20:00 - @@ -66,7 +66,6 @@ struct el_read_t { int read_errno; }; -static int read__fixio(int, int); static int read_char(EditLine *, wchar_t *); static int read_getcmd(EditLine *, el_action_t *, wchar_t *); static voidread_clearmacros(struct macros *); @@ -132,26 +131,6 @@ el_read_getfn(struct el_read_t *el_read) } -/* read__fixio(): - * Try to recover from a read error - */ -static int -read__fixio(int fd, int e) -{ - int zero = 0; - - switch (e) { - case EAGAIN: - if (ioctl(fd, FIONBIO, ) == -1) - return -1; - return 0; - - default: - return -1; - } -} - - /* el_push(): * Push a macro */ @@ -231,15 +210,12 @@ static int read_char(EditLine *el, wchar_t *cp) { ssize_t num_read; - int tried = 0; char cbuf[MB_LEN_MAX]; int cbp = 0; - int save_errno = errno; again: el->el_signal->sig_no = 0; while ((num_read = read(el->el_infd, cbuf + cbp, 1)) == -1) { - int e = errno; if (errno == EINTR) { switch (el->el_signal->sig_no) { case SIGCONT: @@ -252,14 +228,8 @@ read_char(EditLine *el, wchar_t *cp) break; } } - if (!tried && read__fixio(el->el_infd, e) == 0) { - errno = save_errno; - tried = 1; - } else { - errno = e; - *cp = L'\0'; - return -1; - } + *cp = L'\0'; + return -1; } /* Test for EOF */ - 8< - schnipp - >8 - 8< - schnapp - >8 - #include #include #include #include #include int main(void) { EditLine*el; const char *line; int len; int flags = O_NONBLOCK; if (fcntl(STDIN_FILENO, F_SETFL, ) == -1) err(1, "fcntl(F_SETFL)"); flags = fcntl(STDIN_FILENO, F_GETFL); printf("nbio is %s\n", flags & O_NONBLOCK ? "on" : "off"); if ((el = el_init("test", stdin, stdout, stderr)) == NULL) err(1, "el_init"); if ((line = el_gets(el, )) ==