Re: fresh prompt after Ctrl-C in cdio(1)
Ingo Schwarze wrote: > Any user of this program with a sufficient knowledge of signal > handling is welcome to design, implement, and test signal handling > that remains active during other parts of the execution of the > cdio(1) program. The committed patch provides a starting point. Pepole with sufficient knowledge of signal handling are too rare for that to happen.
Re: fresh prompt after Ctrl-C in cdio(1)
Hi, Christian Weisgerber wrote on Thu, Aug 12, 2021 at 06:31:56PM +0200: > Ingo Schwarze: >> 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. >> >> So i propose to do the same in cdio(1), which currently just exits >> on Ctrl-C. >> >> OK? > That looks correct and works fine for me. ok naddy@ Committed for now, thanks for checking. > (I still have a USB CD drive and I use it from time to time to play > audio CDs with cdio. I had completely forgotten that cdio supports > editline, though. The cdio interactive mode is pretty useless since > interrupting a running command aborts the program.) Any user of this program with a sufficient knowledge of signal handling is welcome to design, implement, and test signal handling that remains active during other parts of the execution of the cdio(1) program. The committed patch provides a starting point. Yours, Ingo
Re: fresh prompt after Ctrl-C in cdio(1)
Ingo Schwarze: > 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. > > So i propose to do the same in cdio(1), which currently just exits > on Ctrl-C. > > OK? That looks correct and works fine for me. ok naddy@ (I still have a USB CD drive and I use it from time to time to play audio CDs with cdio. I had completely forgotten that cdio supports editline, though. The cdio interactive mode is pretty useless since interrupting a running command aborts the program.) -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: fresh prompt after Ctrl-C in cdio(1)
Hi Martijn, Martijn van Duren wrote on Thu, Aug 12, 2021 at 04:37:24PM +0200: > 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. More precisely, i restore default signal handling right after, which is "terminate process" for SIGINT. In most situations, i dislike ignoring SIGINT. > 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? Yes. > 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? Maybe. The question whether cdio(1) should allow Ctrl-C to be used to interrupt more operations, and not just command line editing, and return to the interactive prompt in that case is certainly legitimate. Then again, my point is to unify line editing across different programs, not to consider overall signal handling in cdio(1). As long as we only touch line editing in cdio(1), that goal can be reached and the risk of regressions is easier to control. Extending signal handling over larger swaths of code requires reviewing more code and making sure what Ctrl-C does is sane in all code paths in all of that code. For example, looking at the function run(), i see that most commands start by calling open_cd(), but not all do. A few do not call it at all (e.g. CMD_SET, CMD_HELP), in which case we would have to make sure that the signal handler does not remain installed longer than intended, a few do some other work before calling it (e.g. CMD_DEVICE). So i would prefer to leave the broader question of signal handling in cdio(1), outside the specific domain of command line editing, to people who are more familiar with the code and usage of cdio(1). If somebody wants to develop, test and propose a patch with a wider scope, for example a global signal handler that can interupt any operation - such a patch would have to make sure cleanup is done as required after all operations that might get aborted -, i'm not opposed to that, but it don't feel like developing such a patch myself at this point. Yours, Ingo
Re: fresh prompt after Ctrl-C in cdio(1)
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
^C
cdio> cdplay
>
>
> 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 - 1.80
> +++ cdio.c 12 Aug 2021 13:36:38 -
> @@ -63,6 +63,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -158,6 +159,7 @@ int verbose = 1;
> intmsf = 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);
> intplay_next(char *arg);
> intplay_prev(char *arg);
> intplay_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(, 0, sizeof(sa));
> do {
> - if ((buf = (char *) el_gets(el, )) == NULL || !siz) {
> - *cmd = CMD_QUIT;
> + signo = 0;
> + sa.sa_handler = sigint_handler;
> + if (sigaction(SIGINT, , NULL) == -1)
> + err(1, "sigaction");
> + buf = (char *)el_gets(el, );
> + sa.sa_handler = SIG_DFL;
> + if (sigaction(SIGINT, , 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)
>
fresh prompt after Ctrl-C in cdio(1)
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.
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 - 1.80
+++ cdio.c 12 Aug 2021 13:36:38 -
@@ -63,6 +63,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -158,6 +159,7 @@ int verbose = 1;
intmsf = 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);
intplay_next(char *arg);
intplay_prev(char *arg);
intplay_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(, 0, sizeof(sa));
do {
- if ((buf = (char *) el_gets(el, )) == NULL || !siz) {
- *cmd = CMD_QUIT;
+ signo = 0;
+ sa.sa_handler = sigint_handler;
+ if (sigaction(SIGINT, , NULL) == -1)
+ err(1, "sigaction");
+ buf = (char *)el_gets(el, );
+ sa.sa_handler = SIG_DFL;
+ if (sigaction(SIGINT, , 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)
