Re: [Qemu-devel] [PATCH v2 1/3] qemu-io: Add sigraise command

[Max Reitz]

On 2014-12-05 at 13:23, Markus Armbruster wrote:
> Max Reitz <address@hidden> writes:
>
> > On 2014-12-05 at 10:52, 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.
> > > >
> > > > Thus, -c abort is not always useful to simulate a qemu-io crash;
> > > > therefore, this patch adds a new sigraise command which allows to raise
> > > > any Unix signal.
> > > Nitpick: signals are ISO C, not just UNIX.
> > Yes, but "Unix signal" is what the Wikipedia article is named, so... ;-)
> If it's in Wikipedia, it must be right!  Quick, file a bug against the C
> standard!

Yes, I know. But caling it "ISO C signals" or just "C signals" sounds 
strange, since C only defines raise() as far as I remember. Originally, 
I just called it "signals", of course, but that didn't sound so clear. 
"Unix signal" is an unambiguous way to name them.

> > > > Signed-off-by: Max Reitz <address@hidden>
> > > > ---
> > > >    qemu-io-cmds.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> > > >    1 file changed, 46 insertions(+)
> > > >
> > > > diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> > > > index d94fb1e..942b694 100644
> > > > --- a/qemu-io-cmds.c
> > > > +++ b/qemu-io-cmds.c
> > > > @@ -2048,6 +2048,51 @@ static const cmdinfo_t abort_cmd = {
> > > >           .oneline        = "simulate a program crash using abort(3)",
> > > >    };
> > > >    +static void sigraise_help(void)
> > > > +{
> > > > +    printf(
> > > > +"\n"
> > > > +" raises the given Unix signal\n"
> > > > +"\n"
> > > > +" Example:\n"
> > > > +" 'sigraise 9' - raises SIGKILL\n"
> > > Assumes SIGKILL is encoded as 9, which is traditionally the case, but
> > > not actually mandated by POSIX.
> > Yes, I know. The best would be to parse the signal like kill(1) does,
> > but that would have been extra difficult and probably not worth the
> > effort.
> Agree.
>
> > Furthermore, I know there is a song called "kill dash nine", so I
> > guessed it would be enough (at least it'll have to be enough for test
> > 039, thanks to "_supported_os Linux").
> Bash can map signal names to numbers and back:
>
>      $ kill -l 9
>      KILL
>      $ kill -l KILL
>      9
>
> Perhaps you can use that to avoid hardcoding.

I hope you're talking about doing this in the patch and not in qemu-io 
itself. *g*

Well, can do. There is no real reason to as long as the test is only 
supported under Linux anyway, but there's no reason not to do this in 
the respin (other than that I'll lose Fam's R-b...) either. Therefore, 
will do, thanks.

> > > You could avoid hardcoding 9 with
> > >
> > >       " 'sigraise %d' - raises SIGKILL\n"
> > >
> > > with a SIGKILL as argument for %d.
> > Clever. Will do.
> >
> > > But then you'd have to face the fact that SIGKILL is POSIX, not ISO C.
> > > The ISO C signals are SIGABRT, SIGFPE, SIGILL, SIGINT, SIGSEGV, SIGTERM.
> > > Of these, SIGINT and SIGTERM don't dump core, in case you care.
> > Good to know. I guess I'll just go with SIGKILL anyway, it's
> > ubiquitous enough.
> Might break the Windows compile.

Right, the simplest way to do it then would be just to use SIGTERM 
instead of SIGKILL in the example and still use SIGKILL in 039. I'll do 
that if you don't disagree.

Max

> > > > +"\n"
> > > > +" Invokes raise(signal), where \"signal\" is the mandatory integer argument\n"
> > > > +" given to sigraise.\n"
> > > > +"\n");
> > > > +}
> > > > +
> > > > +static int sigraise_f(BlockDriverState *bs, int argc, char **argv);
> > > > +
> > > > +static const cmdinfo_t sigraise_cmd = {
> > > > +    .name       = "sigraise",
> > > > +    .cfunc      = sigraise_f,
> > > > +    .argmin     = 1,
> > > > +    .argmax     = 1,
> > > > +    .flags      = CMD_NOFILE_OK,
> > > > +    .args       = "signal",
> > > > +    .oneline    = "raises a Unix signal",
> > > > +    .help       = sigraise_help,
> > > > +};
> > > > +
> > > > +static int sigraise_f(BlockDriverState *bs, int argc, char **argv)
> > > > +{
> > > > +    int sig = cvtnum(argv[1]);
> > > > +    if (sig < 0) {
> > > > +        printf("non-numeric signal number argument -- %s\n", argv[1]);
> > > > +        return 0;
> > > > +    }
> > > > +
> > > > +    /* Using raise() to kill this process does not necessarily flush all open
> > > > +     * streams. At least stdout and stderr (although the latter should be
> > > > +     * non-buffered anyway) should be flushed, though. */
> > > > +    fflush(stdout);
> > > > +    fflush(stderr);
> > > > +
> > > > +    raise(sig);
> > > > +    return 0;
> > > > +}
> > > > +
> > > >    static void sleep_cb(void *opaque)
> > > >    {
> > > >        bool *expired = opaque;
> > > > @@ -2202,4 +2247,5 @@ static void __attribute((constructor)) 
> > > > init_qemuio_commands(void)
> > > >        qemuio_add_command(&wait_break_cmd);
> > > >        qemuio_add_command(&abort_cmd);
> > > >        qemuio_add_command(&sleep_cmd);
> > > > +    qemuio_add_command(&sigraise_cmd);
> > > >    }
> > > Looks good otherwise.
> > Thanks :-)
> Looking forward to v3 :)