On Thu, 2021-08-12 at 15:57 +0200, Ingo Schwarze wrote:
> Hi,
>
> deraadt@ recently pointed out to me in private mail that it is good
> for usability if interactive programs providing line editing
> functionality are consistent what they do with Ctrl-C, ideally
> discard the current input line and provide a fresh prompt.
> At least that is what bc(1), ftp(1), sftp(1) and all shells do.
>
> So i propose to do the same in cdio(1), which currently just exits
> on Ctrl-C.
>
> Sending to tech@ because i'm not quite sure which developer owns cdio(1),
> or who might be interested. According to the CVS logs, naddy@ was the
> last one to change user-visible functionality, and before that, there
> was nothing but bug fixes and cleanup since 2010.
>
> OK?
> Ingo
>
> P.S.
> Note that the current "!siz" is fragile; el_gets(3) sets it to -1
> on error. I'm polishing that while here.
>
> P.P.S.
> buf = (char *)el_gets(...) is ugly and potentially dangerous, the
> manual page explicitly warns to not do that, but that's a job for
> another day.
>
> P.P.P.S.
> Insects are the most successful class of animals on earth.
> Even in OpenBSD, it is rarely possible to look at code without
> finding something unrelated that could be improved, too.
Maybe I've used cdio once or twice and I don't have a cd-player at hand
(at least connected to my workstation) to test this. So purely from code
inspection: You set the signal handler before entering el_gets and you
ignore it right after.
Wouldn't this imply that if you do something like "cdplay" at the prompt
and while you wait for the cd to spin you hit ^C the application just
exits? If so, wouldn't it make more sense to install the signal handler
once and let open_cd() handle EINTR as well and just return to the prompt?
Something that springs to is
cdio> play
<waiting for spinup><wait, wrong command>^C
cdio> cdplay
<playing your favourite tunes>
>
>
> Index: cdio.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/cdio/cdio.c,v
> retrieving revision 1.80
> diff -u -p -r1.80 cdio.c
> --- cdio.c 18 Jan 2021 00:44:00 -0000 1.80
> +++ cdio.c 12 Aug 2021 13:36:38 -0000
> @@ -63,6 +63,7 @@
> #include <err.h>
> #include <errno.h>
> #include <fcntl.h>
> +#include <signal.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> @@ -158,6 +159,7 @@ int verbose = 1;
> int msf = 1;
> const char *cddb_host;
> char **track_names;
> +volatile sig_atomic_t signo;
>
> EditLine *el = NULL; /* line-editing structure */
> History *hist = NULL; /* line-editing history */
> @@ -179,6 +181,7 @@ int pstatus(char *arg);
> int play_next(char *arg);
> int play_prev(char *arg);
> int play_same(char *arg);
> +void sigint_handler(int);
> char *input(int *);
> char *prompt(void);
> void prtrack(struct cd_toc_entry *e, int lastflag, char *name);
> @@ -1499,18 +1502,36 @@ status(int *trk, int *min, int *sec, int
> return s.data->header.audio_status;
> }
>
> +void
> +sigint_handler(int signo_arg)
> +{
> + signo = signo_arg;
> +}
> +
> char *
> input(int *cmd)
> {
> + struct sigaction sa;
> char *buf;
> int siz = 0;
> char *p;
> HistEvent hev;
>
> + memset(&sa, 0, sizeof(sa));
> do {
> - if ((buf = (char *) el_gets(el, &siz)) == NULL || !siz) {
> - *cmd = CMD_QUIT;
> + signo = 0;
> + sa.sa_handler = sigint_handler;
> + if (sigaction(SIGINT, &sa, NULL) == -1)
> + err(1, "sigaction");
> + buf = (char *)el_gets(el, &siz);
> + sa.sa_handler = SIG_DFL;
> + if (sigaction(SIGINT, &sa, NULL) == -1)
> + err(1, "sigaction");
> + if (buf == NULL || siz <= 0) {
> fprintf(stderr, "\r\n");
> + if (siz < 0 && errno == EINTR && signo == SIGINT)
> + continue;
> + *cmd = CMD_QUIT;
> return (0);
> }
> if (strlen(buf) > 1)
>
