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

[Max Reitz <mreitz@redhat.com>]

On 2014-11-25 at 14:19, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> 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 <mreitz@redhat.com>
>> ---
>>   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