From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40696) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtGDi-0005dp-4p for qemu-devel@nongnu.org; Tue, 25 Nov 2014 08:32:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XtGDa-0001ci-Kc for qemu-devel@nongnu.org; Tue, 25 Nov 2014 08:31:54 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59137) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtGDa-0001ce-Ch for qemu-devel@nongnu.org; Tue, 25 Nov 2014 08:31:46 -0500 Message-ID: <547484BC.4080500@redhat.com> Date: Tue, 25 Nov 2014 14:31:40 +0100 From: Max Reitz MIME-Version: 1.0 References: <1416910178-5088-1-git-send-email-mreitz@redhat.com> <1416910178-5088-2-git-send-email-mreitz@redhat.com> <87tx1nfmea.fsf@blackfin.pond.sub.org> In-Reply-To: <87tx1nfmea.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/3] qemu-io: Let -c abort raise any signal List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Kevin Wolf , Fam Zheng , =?ISO-8859-15?Q?Michael_M=FCller?= , Mao Chuan Li , qemu-devel@nongnu.org, Stefan Hajnoczi On 2014-11-25 at 14:19, Markus Armbruster wrote: > Max Reitz 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 >> --- >> 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