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 -0000 1.46
> +++ editline.3 11 Aug 2021 16:20:00 -0000
> @@ -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 -0000 1.47
> +++ read.c 11 Aug 2021 16:20:00 -0000
> @@ -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 void read_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, &zero) == -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 <err.h>
> #include <fcntl.h>
> #include <histedit.h>
> #include <stdio.h>
> #include <unistd.h>
>
> int
> main(void)
> {
> EditLine *el;
> const char *line;
> int len;
> int flags = O_NONBLOCK;
>
> if (fcntl(STDIN_FILENO, F_SETFL, &flags) == -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, &len)) == NULL)
> err(1, "el_gets");
> printf("the user said: %.*s", len, line);
> flags = fcntl(STDIN_FILENO, F_GETFL);
> printf("nbio is %s\n", flags & O_NONBLOCK ? "on" : "off");
> return 0;
> }
>
