Btw, would there be any benefit to declare zero as const in this
context?
On Sun, 2021-08-08 at 13:42 +0200, Ingo Schwarze wrote:
> Hi,
>
> deraadt@ recently reported to me that the editline(3) library, while
> line editing is active - for example inside el_gets(3) - ignores
> the first SIGINT it receives, for example the the first Ctrl-C the
> user presses. I consider that a bug in the editline(3) library.
> Some programs, for example our old ftp(1) implementation, work
> around the bug in a horrible way by using setjmp(3)/longjmp(3).
>
> The root cause of the problem is in libedit/read.c, in the interaction
> of the functions read_char() and read__fixio(). Before fixing the bug
> can reasonably be considered, the function read__fixio() direly needs
> cleanup. As it stands, it is utterly unreadable.
>
> So here is a patch to make it clear what the function does, with
> no functional change intended yet (-37 +5 LOC).
>
> There will be one or more follow-up patches. If you want to receive
> them, please reply to me, i won't necessarily send them all to
> tech@.
>
> I see some value in avoiding gratuitious divergence from NetBSD,
> but getting rid of this horrible mess is not gratuitious.
>
> Rationale:
> * Do not mark an argument as unused that is actually used.
> * errno cannot possibly be -1. Even is it were, treating it as
> EAGAIN makes no sense, treating it as the most severe error
> imaginable makes more sense to me.
> * We define EWOULDBLOCK to be the same as EAGAIN, so no need
> to handle it separately.
> * No need to #define TRY_AGAIN to use it just once.
> * Don't do the same thing twice. We do support the FIONBIO ioctl(2),
> so the the indirection using the F_GETFL fcntl(2) can be deleted.
> * No need to play confusing games with the "e" variable.
> Just return -1 for error or 0 for success in a straightforward
> manner.
>
> OK?
> Ingo
>
> P.S.
> I also considered whether this FIONBIO dance should better be done
> at the initialization stage rather than after EAGAIN already happened.
> But maybe not. This is a library. The application program might
> set the fd to non-blocking mode at any time and then call el_gets(3)
> again, in which case the library needs to restore blocking I/O to
> do its work.
>
>
> Index: read.c
> ===================================================================
> RCS file: /cvs/src/lib/libedit/read.c,v
> retrieving revision 1.44
> diff -u -p -r1.44 read.c
> --- read.c 25 May 2016 09:36:21 -0000 1.44
> +++ read.c 8 Aug 2021 10:30:06 -0000
> @@ -39,9 +39,10 @@
> * read.c: Clean this junk up! This is horrible code.
> * Terminal read functions
> */
> +#include <sys/ioctl.h>
> +
> #include <ctype.h>
> #include <errno.h>
> -#include <fcntl.h>
> #include <limits.h>
> #include <stdlib.h>
> #include <string.h>
> @@ -134,55 +135,16 @@ el_read_getfn(struct el_read_t *el_read)
> /* read__fixio():
> * Try to recover from a read error
> */
> -/* ARGSUSED */
> static int
> -read__fixio(int fd __attribute__((__unused__)), int e)
> +read__fixio(int fd, int e)
> {
> + int zero = 0;
>
> switch (e) {
> - case -1: /* Make sure that the code is reachable */
> -
> -#ifdef EWOULDBLOCK
> - case EWOULDBLOCK:
> -#ifndef TRY_AGAIN
> -#define TRY_AGAIN
> -#endif
> -#endif /* EWOULDBLOCK */
> -
> -#if defined(POSIX) && defined(EAGAIN)
> -#if defined(EWOULDBLOCK) && EWOULDBLOCK != EAGAIN
> case EAGAIN:
> -#ifndef TRY_AGAIN
> -#define TRY_AGAIN
> -#endif
> -#endif /* EWOULDBLOCK && EWOULDBLOCK != EAGAIN */
> -#endif /* POSIX && EAGAIN */
> -
> - e = 0;
> -#ifdef TRY_AGAIN
> -#if defined(F_SETFL) && defined(O_NDELAY)
> - if ((e = fcntl(fd, F_GETFL)) == -1)
> + if (ioctl(fd, FIONBIO, &zero) == -1)
> return -1;
> -
> - if (fcntl(fd, F_SETFL, e & ~O_NDELAY) == -1)
> - return -1;
> - else
> - e = 1;
> -#endif /* F_SETFL && O_NDELAY */
> -
> -#ifdef FIONBIO
> - {
> - int zero = 0;
> -
> - if (ioctl(fd, FIONBIO, &zero) == -1)
> - return -1;
> - else
> - e = 1;
> - }
> -#endif /* FIONBIO */
> -
> -#endif /* TRY_AGAIN */
> - return e ? 0 : -1;
> + return 0;
>
> case EINTR:
> return 0;
>
