Re: libedit: read__fixio cleanup
Hi Martijn, Martijn van Duren wrote on Mon, Aug 09, 2021 at 11:04:35AM +0200: > Btw, would there be any benefit to declare zero as const in this > context? I dont think so. At least according to the ioctl(2) manual page, FIONBIO expects an int * argument, not a const int *. Yours, Ingo
Re: libedit: read__fixio cleanup
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 - 1.44
> +++ read.c 8 Aug 2021 10:30:06 -
> @@ -39,9 +39,10 @@
> * read.c: Clean this junk up! This is horrible code.
> * Terminal read functions
> */
> +#include
> +
> #include
> #include
> -#include
> #include
> #include
> #include
> @@ -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, ) == -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, ) == -1)
> - return -1;
> - else
> - e = 1;
> - }
> -#endif /* FIONBIO */
> -
> -#endif /* TRY_AGAIN */
> - return e ? 0 : -1;
> + return 0;
>
> case EINTR:
> return 0;
>
Re: libedit: read__fixio cleanup
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.
>
Hopefully without getting dragged too much into this one:
Your reasoning and diff look fine to me.
OK martijn@
>
> 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 - 1.44
> +++ read.c 8 Aug 2021 10:30:06 -
> @@ -39,9 +39,10 @@
> * read.c: Clean this junk up! This is horrible code.
> * Terminal read functions
> */
> +#include
> +
> #include
> #include
> -#include
> #include
> #include
> #include
> @@ -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, ) == -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, ) == -1)
> - return -1;
> - else
> - e = 1;
> - }
> -#endif /* FIONBIO */
> -
> -#endif /* TRY_AGAIN */
> - return e ? 0 : -1;
> + return 0;
>
> case EINTR:
> return 0;
>
libedit: read__fixio cleanup
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 - 1.44
+++ read.c 8 Aug 2021 10:30:06 -
@@ -39,9 +39,10 @@
* read.c: Clean this junk up! This is horrible code.
*Terminal read functions
*/
+#include
+
#include
#include
-#include
#include
#include
#include
@@ -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, ) == -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, ) == -1)
- return -1;
- else
- e = 1;
- }
-#endif /* FIONBIO */
-
-#endif /* TRY_AGAIN */
- return e ? 0 : -1;
+ return 0;
case EINTR:
return 0;
