Re: [Qemu-devel] [PATCH 1/3] qemu-io: Let -c abort raise any signal

[Max Reitz]

On 2014-11-25 at 14:19, Markus Armbruster wrote:
> Max Reitz <address@hidden> writes:
>
> > abort() has the sometimes undesirable side-effect of generating a core
> > dump. If that is not needed, SIGKILL has the same effect of abruptly
> > crash qemu; without a core dump.
> >
> > Therefore, this patch allows to use the qemu-io abort command to raise
> > any signal.
> >
> > Signed-off-by: Max Reitz <address@hidden>
> > ---
> >   qemu-io-cmds.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> >   1 file changed, 56 insertions(+), 3 deletions(-)
> >
> > diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> > index d94fb1e..5d39cf4 100644
> > --- a/qemu-io-cmds.c
> > +++ b/qemu-io-cmds.c
> > @@ -2036,18 +2036,71 @@ static const cmdinfo_t wait_break_cmd = {
> >          .oneline        = "waits for the suspension of a request",
> >   };
> >   
> > -static int abort_f(BlockDriverState *bs, int argc, char **argv)
> > +
> > +static void abort_help(void)
> >   {
> > -    abort();
> > +    printf(
> > +"\n"
> > +" simulates a program crash\n"
> > +"\n"
> > +" Invokes abort(), or raise(signal) if a signal number is specified.\n"
> > +" -S, -- number of the signal to raise()\n"
> > +"\n");
> >   }
> >   
> > +static int abort_f(BlockDriverState *bs, int argc, char **argv);
> > +
> >   static const cmdinfo_t abort_cmd = {
> >          .name           = "abort",
> >          .cfunc          = abort_f,
> > +       .argmin         = 0,
> > +       .argmax         = 2,
> >          .flags          = CMD_NOFILE_OK,
> > -       .oneline        = "simulate a program crash using abort(3)",
> > +       .args           = "[-S signal]",
> > +       .oneline        = "simulate a program crash",
> > +       .help           = abort_help,
> >   };
> This overloads the abort command with a kill command.

abort() does a bit more than raise(SIGABRT), but all that it does more 
is basically "Make sure that raise(SIGABRT) actually works". So abort() 
basically is already a "kill me" command (kill -SIGABRT). I think 
overloading it is fine, but I wouldn't have that much of a problem to 
introduce another command, but it was just simpler this way.

> Do we really need a way to send arbitrary signals?

Why not? I'm implementing the functionality here for a single signal, 
it's not that hard to do it for arbitrary signals, so I did it.

> If yes, shouldn't we
> call it "kill" rather than "abort"?

I'd call it "raise" (for obious reasons), but will not rename "abort". I 
can create a separate "raise" or "kill" command, though, obviously. But 
as "abort" is basically a "raise 6" (or "abort -S 6" with this version 
of the series), I think it's completely fine to overload "abort".

> I suspect fooling around with signals isn't necessary, and a simple
> exit(1) would do.

No, because that would execute the atexit() functions. I don't know 
whether such are used in qemu, but if we want to simulate a crash, 
exit() is not the right function to do that.

> >   
> > +static int abort_f(BlockDriverState *bs, int argc, char **argv)
> > +{
> > +    int c;
> > +    int sig = -1;
> > +
> > +    while ((c = getopt(argc, argv, "S:")) != EOF) {
> > +        switch (c) {
> > +            case 'S':
> > +                sig = cvtnum(optarg);
> > +                if (sig < 0) {
> > +                    printf("non-numeric signal number argument -- %s\n", 
> > optarg);
> > +                    return 0;
> > +                }
> > +                break;
> > +
> > +            default:
> > +                return qemuio_command_usage(&abort_cmd);
> > +        }
> > +    }
> > +
> > +    if (optind != argc) {
> > +        return qemuio_command_usage(&abort_cmd);
> > +    }
> > +
> > +    if (sig < 0) {
> > +        abort();
> > +    } else {
> > +        /* While abort() does flush all open streams, using raise() to kill 
> > this
> > +         * process does not necessarily. At least stdout and stderr (although
> > +         * the latter should be non-buffered anyway) should be flushed, though.
> > +         */
> > +        fflush(stdout);
> > +        fflush(stderr);
> Without -S, we flush all streams.  With -S, we flush only stdout and
> stderr.  The inconsistency is ugly.  Could be avoided with fcloseall(),
> except it's a GNU extension.  Or drop the non-signal path entirely, and
> raise(SIGABRT) instead of abort().

Except abort() does a bit more.

Because I think not flushing any stream except for stdout and stderr is 
closer to a real crash, I think making sig = 6 the default and thus 
dropping the non-signal path is the better option.

Max