* [Qemu-devel] [PATCH 0/3] iotests: Fix test 039
@ 2014-11-25 10:09 Max Reitz
2014-11-25 10:09 ` [Qemu-devel] [PATCH 1/3] qemu-io: Let -c abort raise any signal Max Reitz
` (4 more replies)
0 siblings, 5 replies; 17+ messages in thread
From: Max Reitz @ 2014-11-25 10:09 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Michael Müller, Mao Chuan Li,
Max Reitz, Stefan Hajnoczi
Test 039 used to fail because qemu-io -c abort may generate core dumps
even with ulimit -c 0 (and the output then contains "(core dumped)").
Fix this by adding an option to the "abort" command which allows
specifying a signal number to raise(). Using SIGKILL for example does
not result in a core dump, but it still badly crashes qemu-io (as
desired).
I am sending this series because we need all tests to work before adding
the check-block target to "make check" (which we will hopefully do
soon).
Max Reitz (3):
qemu-io: Let -c abort raise any signal
iotests: Filter for "Killed" in qemu-io output
iotests: Fix test 039
qemu-io-cmds.c | 59 ++++++++++++++++++++++++++++++++++++++--
tests/qemu-iotests/039 | 12 ++++----
tests/qemu-iotests/039.out | 6 ++--
tests/qemu-iotests/common.filter | 2 +-
4 files changed, 67 insertions(+), 12 deletions(-)
--
1.9.3
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 1/3] qemu-io: Let -c abort raise any signal
2014-11-25 10:09 [Qemu-devel] [PATCH 0/3] iotests: Fix test 039 Max Reitz
@ 2014-11-25 10:09 ` Max Reitz
2014-11-25 13:19 ` Markus Armbruster
2014-11-25 10:09 ` [Qemu-devel] [PATCH 2/3] iotests: Filter for "Killed" in qemu-io output Max Reitz
` (3 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Max Reitz @ 2014-11-25 10:09 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Michael Müller, Mao Chuan Li,
Max Reitz, Stefan Hajnoczi
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,
};
+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);
+
+ raise(sig);
+ /* raise() may return */
+ return 0;
+ }
+}
+
static void sleep_cb(void *opaque)
{
bool *expired = opaque;
--
1.9.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 2/3] iotests: Filter for "Killed" in qemu-io output
2014-11-25 10:09 [Qemu-devel] [PATCH 0/3] iotests: Fix test 039 Max Reitz
2014-11-25 10:09 ` [Qemu-devel] [PATCH 1/3] qemu-io: Let -c abort raise any signal Max Reitz
@ 2014-11-25 10:09 ` Max Reitz
2014-11-25 13:05 ` Markus Armbruster
2014-11-25 10:09 ` [Qemu-devel] [PATCH 3/3] iotests: Fix test 039 Max Reitz
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Max Reitz @ 2014-11-25 10:09 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Michael Müller, Mao Chuan Li,
Max Reitz, Stefan Hajnoczi
_filter_qemu_io already filters out the process ID when qemu-io is
aborted; the same should be done if it is aborted.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/common.filter | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index dfb9726..6c14590 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -150,7 +150,7 @@ _filter_win32()
_filter_qemu_io()
{
_filter_win32 | sed -e "s/[0-9]* ops\; [0-9/:. sec]* ([0-9/.inf]* [EPTGMKiBbytes]*\/sec and [0-9/.inf]* ops\/sec)/X ops\; XX:XX:XX.X (XXX YYY\/sec and XXX ops\/sec)/" \
- -e "s/: line [0-9][0-9]*: *[0-9][0-9]*\( Aborted\)/:\1/" \
+ -e "s/: line [0-9][0-9]*: *[0-9][0-9]*\( Aborted\| Killed\)/:\1/" \
-e "s/qemu-io> //g"
}
--
1.9.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 3/3] iotests: Fix test 039
2014-11-25 10:09 [Qemu-devel] [PATCH 0/3] iotests: Fix test 039 Max Reitz
2014-11-25 10:09 ` [Qemu-devel] [PATCH 1/3] qemu-io: Let -c abort raise any signal Max Reitz
2014-11-25 10:09 ` [Qemu-devel] [PATCH 2/3] iotests: Filter for "Killed" in qemu-io output Max Reitz
@ 2014-11-25 10:09 ` Max Reitz
2014-11-25 12:21 ` [Qemu-devel] [PATCH 0/3] " Markus Armbruster
2014-11-25 16:48 ` Michael Mueller
4 siblings, 0 replies; 17+ messages in thread
From: Max Reitz @ 2014-11-25 10:09 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Michael Müller, Mao Chuan Li,
Max Reitz, Stefan Hajnoczi
Test 039 used qemu-io -c abort for simulating a qemu crash; however,
abort() generally results in a core dump and ulimit -c 0 is no reliable
way of preventing that. Use "abort -S 9" instead to have it crash
without a core dump.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/039 | 12 +++++++-----
tests/qemu-iotests/039.out | 6 +++---
2 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
index 84c9167..813822c 100755
--- a/tests/qemu-iotests/039
+++ b/tests/qemu-iotests/039
@@ -47,9 +47,11 @@ _supported_os Linux
_default_cache_mode "writethrough"
_supported_cache_modes "writethrough"
-_no_dump_exec()
+_subshell_exec()
{
- (ulimit -c 0; exec "$@")
+ # Executing crashing commands in a subshell prevents information like the
+ # "Killed" line from being lost
+ (exec "$@")
}
size=128M
@@ -72,7 +74,7 @@ echo "== Creating a dirty image file =="
IMGOPTS="compat=1.1,lazy_refcounts=on"
_make_test_img $size
-_no_dump_exec $QEMU_IO -c "write -P 0x5a 0 512" -c "abort" "$TEST_IMG" 2>&1 | _filter_qemu_io
+_subshell_exec $QEMU_IO -c "write -P 0x5a 0 512" -c "abort -S 9" "$TEST_IMG" 2>&1 | _filter_qemu_io
# The dirty bit must be set
$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
@@ -105,7 +107,7 @@ echo "== Opening a dirty image read/write should repair it =="
IMGOPTS="compat=1.1,lazy_refcounts=on"
_make_test_img $size
-_no_dump_exec $QEMU_IO -c "write -P 0x5a 0 512" -c "abort" "$TEST_IMG" 2>&1 | _filter_qemu_io
+_subshell_exec $QEMU_IO -c "write -P 0x5a 0 512" -c "abort -S 9" "$TEST_IMG" 2>&1 | _filter_qemu_io
# The dirty bit must be set
$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
@@ -121,7 +123,7 @@ echo "== Creating an image file with lazy_refcounts=off =="
IMGOPTS="compat=1.1,lazy_refcounts=off"
_make_test_img $size
-_no_dump_exec $QEMU_IO -c "write -P 0x5a 0 512" -c "abort" "$TEST_IMG" 2>&1 | _filter_qemu_io
+_subshell_exec $QEMU_IO -c "write -P 0x5a 0 512" -c "abort -S 9" "$TEST_IMG" 2>&1 | _filter_qemu_io
# The dirty bit must not be set since lazy_refcounts=off
$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
index 0adf153..35a04bd 100644
--- a/tests/qemu-iotests/039.out
+++ b/tests/qemu-iotests/039.out
@@ -11,7 +11,7 @@ No errors were found on the image.
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
wrote 512/512 bytes at offset 0
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-./039: Aborted ( ulimit -c 0; exec "$@" )
+./039: Killed ( exec "$@" )
incompatible_features 0x1
ERROR cluster 5 refcount=0 reference=1
ERROR OFLAG_COPIED data cluster: l2_entry=8000000000050000 refcount=0
@@ -46,7 +46,7 @@ read 512/512 bytes at offset 0
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
wrote 512/512 bytes at offset 0
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-./039: Aborted ( ulimit -c 0; exec "$@" )
+./039: Killed ( exec "$@" )
incompatible_features 0x1
ERROR cluster 5 refcount=0 reference=1
Rebuilding refcount structure
@@ -60,7 +60,7 @@ incompatible_features 0x0
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
wrote 512/512 bytes at offset 0
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-./039: Aborted ( ulimit -c 0; exec "$@" )
+./039: Killed ( exec "$@" )
incompatible_features 0x0
No errors were found on the image.
--
1.9.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] iotests: Fix test 039
2014-11-25 10:09 [Qemu-devel] [PATCH 0/3] iotests: Fix test 039 Max Reitz
` (2 preceding siblings ...)
2014-11-25 10:09 ` [Qemu-devel] [PATCH 3/3] iotests: Fix test 039 Max Reitz
@ 2014-11-25 12:21 ` Markus Armbruster
2014-11-25 12:29 ` Max Reitz
2014-11-25 16:48 ` Michael Mueller
4 siblings, 1 reply; 17+ messages in thread
From: Markus Armbruster @ 2014-11-25 12:21 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Fam Zheng, Michael Müller, Mao Chuan Li,
qemu-devel, Stefan Hajnoczi
Max Reitz <mreitz@redhat.com> writes:
> Test 039 used to fail
I'm confused: "used to" suggests it doesn't anymore, but you sending a
patches strongly suggests something's broken.
> because qemu-io -c abort may generate core dumps
> even with ulimit -c 0 (and the output then contains "(core dumped)").
How?
[...]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] iotests: Fix test 039
2014-11-25 12:21 ` [Qemu-devel] [PATCH 0/3] " Markus Armbruster
@ 2014-11-25 12:29 ` Max Reitz
2014-11-25 13:20 ` Markus Armbruster
0 siblings, 1 reply; 17+ messages in thread
From: Max Reitz @ 2014-11-25 12:29 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Fam Zheng, Michael Müller, Mao Chuan Li,
qemu-devel, Stefan Hajnoczi
On 2014-11-25 at 13:21, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
>
>> Test 039 used to fail
> I'm confused: "used to" suggests it doesn't anymore, but you sending a
> patches strongly suggests something's broken.
Well, it used to fail before this series. :-P
You're right, this sounds bad. Currently, 039 does fail, at least on any
system with a /proc/sys/kernel/core_pattern passing the dump to another
program. After this series, it does no longer.
>> because qemu-io -c abort may generate core dumps
>> even with ulimit -c 0 (and the output then contains "(core dumped)").
> How?
See the patches[1][2] by Mao Chuan Li. If /proc/sys/kernel/core_pattern
passes the dump to another program, ulimit -c 0 does not matter.
[1] http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg02092.html
[2] http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg02093.html
The problem with those patches is that they require access to
/proc/sys/kernel/core_pattern. I don't like having to run the iotests as
root.
Max
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] iotests: Filter for "Killed" in qemu-io output
2014-11-25 10:09 ` [Qemu-devel] [PATCH 2/3] iotests: Filter for "Killed" in qemu-io output Max Reitz
@ 2014-11-25 13:05 ` Markus Armbruster
2014-11-25 13:06 ` Max Reitz
0 siblings, 1 reply; 17+ messages in thread
From: Markus Armbruster @ 2014-11-25 13:05 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Fam Zheng, Michael Müller, Mao Chuan Li,
qemu-devel, Stefan Hajnoczi
Max Reitz <mreitz@redhat.com> writes:
> _filter_qemu_io already filters out the process ID when qemu-io is
> aborted; the same should be done if it is aborted.
"when it is killed".
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] iotests: Filter for "Killed" in qemu-io output
2014-11-25 13:05 ` Markus Armbruster
@ 2014-11-25 13:06 ` Max Reitz
0 siblings, 0 replies; 17+ messages in thread
From: Max Reitz @ 2014-11-25 13:06 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Fam Zheng, Michael Müller, Mao Chuan Li,
qemu-devel, Stefan Hajnoczi
On 2014-11-25 at 14:05, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
>
>> _filter_qemu_io already filters out the process ID when qemu-io is
>> aborted; the same should be done if it is aborted.
> "when it is killed".
Oops, right.
Max
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] qemu-io: Let -c abort raise any signal
2014-11-25 10:09 ` [Qemu-devel] [PATCH 1/3] qemu-io: Let -c abort raise any signal Max Reitz
@ 2014-11-25 13:19 ` Markus Armbruster
2014-11-25 13:31 ` Max Reitz
0 siblings, 1 reply; 17+ messages in thread
From: Markus Armbruster @ 2014-11-25 13:19 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Fam Zheng, Michael Müller, Mao Chuan Li,
qemu-devel, Stefan Hajnoczi
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.
Do we really need a way to send arbitrary signals? If yes, shouldn't we
call it "kill" rather than "abort"?
I suspect fooling around with signals isn't necessary, and a simple
exit(1) would do.
>
> +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().
> +
> + raise(sig);
> + /* raise() may return */
> + return 0;
> + }
> +}
> +
> static void sleep_cb(void *opaque)
> {
> bool *expired = opaque;
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] iotests: Fix test 039
2014-11-25 12:29 ` Max Reitz
@ 2014-11-25 13:20 ` Markus Armbruster
2014-11-25 13:22 ` Max Reitz
0 siblings, 1 reply; 17+ messages in thread
From: Markus Armbruster @ 2014-11-25 13:20 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Fam Zheng, Michael Müller, Mao Chuan Li,
qemu-devel, Stefan Hajnoczi
Max Reitz <mreitz@redhat.com> writes:
> On 2014-11-25 at 13:21, Markus Armbruster wrote:
>> Max Reitz <mreitz@redhat.com> writes:
>>
>>> Test 039 used to fail
>> I'm confused: "used to" suggests it doesn't anymore, but you sending a
>> patches strongly suggests something's broken.
>
> Well, it used to fail before this series. :-P
>
> You're right, this sounds bad. Currently, 039 does fail, at least on
> any system with a /proc/sys/kernel/core_pattern passing the dump to
> another program. After this series, it does no longer.
>
>>> because qemu-io -c abort may generate core dumps
>>> even with ulimit -c 0 (and the output then contains "(core dumped)").
>> How?
>
> See the patches[1][2] by Mao Chuan Li. If
> /proc/sys/kernel/core_pattern passes the dump to another program,
> ulimit -c 0 does not matter.
>
> [1] http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg02092.html
> [2] http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg02093.html
>
> The problem with those patches is that they require access to
> /proc/sys/kernel/core_pattern. I don't like having to run the iotests
> as root.
To me, this sounds like a case of "doctor, it hurts when I do this".
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] iotests: Fix test 039
2014-11-25 13:20 ` Markus Armbruster
@ 2014-11-25 13:22 ` Max Reitz
2014-11-25 13:48 ` Markus Armbruster
0 siblings, 1 reply; 17+ messages in thread
From: Max Reitz @ 2014-11-25 13:22 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Fam Zheng, Michael Müller, Mao Chuan Li,
qemu-devel, Stefan Hajnoczi
On 2014-11-25 at 14:20, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
>
>> On 2014-11-25 at 13:21, Markus Armbruster wrote:
>>> Max Reitz <mreitz@redhat.com> writes:
>>>
>>>> Test 039 used to fail
>>> I'm confused: "used to" suggests it doesn't anymore, but you sending a
>>> patches strongly suggests something's broken.
>> Well, it used to fail before this series. :-P
>>
>> You're right, this sounds bad. Currently, 039 does fail, at least on
>> any system with a /proc/sys/kernel/core_pattern passing the dump to
>> another program. After this series, it does no longer.
>>
>>>> because qemu-io -c abort may generate core dumps
>>>> even with ulimit -c 0 (and the output then contains "(core dumped)").
>>> How?
>> See the patches[1][2] by Mao Chuan Li. If
>> /proc/sys/kernel/core_pattern passes the dump to another program,
>> ulimit -c 0 does not matter.
>>
>> [1] http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg02092.html
>> [2] http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg02093.html
>>
>> The problem with those patches is that they require access to
>> /proc/sys/kernel/core_pattern. I don't like having to run the iotests
>> as root.
> To me, this sounds like a case of "doctor, it hurts when I do this".
What do you mean? That I don't want the iotests to run as root? Or that
I don't want to go the alternative of filtering out the "(core dumped)"
message?
039 doesn't need a core dump. abort() generates a core dump. Why are we
using abort()? Makes no sense. So don't use it.
I can see that people may want to use qemu-io -c abort to generate a
core dump, though, which is why I'm not simply replacing the abort()
call by raise(SIGKILL).
Max
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] qemu-io: Let -c abort raise any signal
2014-11-25 13:19 ` Markus Armbruster
@ 2014-11-25 13:31 ` Max Reitz
0 siblings, 0 replies; 17+ messages in thread
From: Max Reitz @ 2014-11-25 13:31 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Fam Zheng, Michael Müller, Mao Chuan Li,
qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] iotests: Fix test 039
2014-11-25 13:22 ` Max Reitz
@ 2014-11-25 13:48 ` Markus Armbruster
2014-11-25 13:50 ` Max Reitz
0 siblings, 1 reply; 17+ messages in thread
From: Markus Armbruster @ 2014-11-25 13:48 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Fam Zheng, Michael Müller, Mao Chuan Li,
qemu-devel, Stefan Hajnoczi
Max Reitz <mreitz@redhat.com> writes:
> On 2014-11-25 at 14:20, Markus Armbruster wrote:
>> Max Reitz <mreitz@redhat.com> writes:
>>
>>> On 2014-11-25 at 13:21, Markus Armbruster wrote:
>>>> Max Reitz <mreitz@redhat.com> writes:
>>>>
>>>>> Test 039 used to fail
>>>> I'm confused: "used to" suggests it doesn't anymore, but you sending a
>>>> patches strongly suggests something's broken.
>>> Well, it used to fail before this series. :-P
>>>
>>> You're right, this sounds bad. Currently, 039 does fail, at least on
>>> any system with a /proc/sys/kernel/core_pattern passing the dump to
>>> another program. After this series, it does no longer.
>>>
>>>>> because qemu-io -c abort may generate core dumps
>>>>> even with ulimit -c 0 (and the output then contains "(core dumped)").
>>>> How?
>>> See the patches[1][2] by Mao Chuan Li. If
>>> /proc/sys/kernel/core_pattern passes the dump to another program,
>>> ulimit -c 0 does not matter.
>>>
>>> [1] http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg02092.html
>>> [2] http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg02093.html
>>>
>>> The problem with those patches is that they require access to
>>> /proc/sys/kernel/core_pattern. I don't like having to run the iotests
>>> as root.
>> To me, this sounds like a case of "doctor, it hurts when I do this".
>
> What do you mean? That I don't want the iotests to run as root? Or
> that I don't want to go the alternative of filtering out the "(core
> dumped)" message?
I mean:
Doctor, it hurts when I write weird stuff to
/proc/sys/kernel/core_pattern.
Don't do that then.
If you want to be a nicer doc than me, go right ahead.
[...]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] iotests: Fix test 039
2014-11-25 13:48 ` Markus Armbruster
@ 2014-11-25 13:50 ` Max Reitz
2014-11-25 14:04 ` Markus Armbruster
0 siblings, 1 reply; 17+ messages in thread
From: Max Reitz @ 2014-11-25 13:50 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Fam Zheng, Michael Müller, Mao Chuan Li,
qemu-devel, Stefan Hajnoczi
On 2014-11-25 at 14:48, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
>
>> On 2014-11-25 at 14:20, Markus Armbruster wrote:
>>> Max Reitz <mreitz@redhat.com> writes:
>>>
>>>> On 2014-11-25 at 13:21, Markus Armbruster wrote:
>>>>> Max Reitz <mreitz@redhat.com> writes:
>>>>>
>>>>>> Test 039 used to fail
>>>>> I'm confused: "used to" suggests it doesn't anymore, but you sending a
>>>>> patches strongly suggests something's broken.
>>>> Well, it used to fail before this series. :-P
>>>>
>>>> You're right, this sounds bad. Currently, 039 does fail, at least on
>>>> any system with a /proc/sys/kernel/core_pattern passing the dump to
>>>> another program. After this series, it does no longer.
>>>>
>>>>>> because qemu-io -c abort may generate core dumps
>>>>>> even with ulimit -c 0 (and the output then contains "(core dumped)").
>>>>> How?
>>>> See the patches[1][2] by Mao Chuan Li. If
>>>> /proc/sys/kernel/core_pattern passes the dump to another program,
>>>> ulimit -c 0 does not matter.
>>>>
>>>> [1] http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg02092.html
>>>> [2] http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg02093.html
>>>>
>>>> The problem with those patches is that they require access to
>>>> /proc/sys/kernel/core_pattern. I don't like having to run the iotests
>>>> as root.
>>> To me, this sounds like a case of "doctor, it hurts when I do this".
>> What do you mean? That I don't want the iotests to run as root? Or
>> that I don't want to go the alternative of filtering out the "(core
>> dumped)" message?
> I mean:
>
> Doctor, it hurts when I write weird stuff to
> /proc/sys/kernel/core_pattern.
>
> Don't do that then.
>
> If you want to be a nicer doc than me, go right ahead.
I don't write weird stuff there. My default system configuration does
(and mine is not the only one):
$ uname -r
3.17.3-200.fc20.x86_64
$ cat /proc/sys/kernel/core_pattern
|/usr/sbin/chroot /proc/%P/root /usr/libexec/abrt-hook-ccpp %s %c %p %u
%g %t e
Max
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] iotests: Fix test 039
2014-11-25 13:50 ` Max Reitz
@ 2014-11-25 14:04 ` Markus Armbruster
2014-11-25 16:53 ` Michael Mueller
0 siblings, 1 reply; 17+ messages in thread
From: Markus Armbruster @ 2014-11-25 14:04 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Fam Zheng, Michael Müller, Mao Chuan Li,
qemu-devel, Stefan Hajnoczi
Max Reitz <mreitz@redhat.com> writes:
> On 2014-11-25 at 14:48, Markus Armbruster wrote:
>> Max Reitz <mreitz@redhat.com> writes:
>>
>>> On 2014-11-25 at 14:20, Markus Armbruster wrote:
>>>> Max Reitz <mreitz@redhat.com> writes:
>>>>
>>>>> On 2014-11-25 at 13:21, Markus Armbruster wrote:
>>>>>> Max Reitz <mreitz@redhat.com> writes:
>>>>>>
>>>>>>> Test 039 used to fail
>>>>>> I'm confused: "used to" suggests it doesn't anymore, but you sending a
>>>>>> patches strongly suggests something's broken.
>>>>> Well, it used to fail before this series. :-P
>>>>>
>>>>> You're right, this sounds bad. Currently, 039 does fail, at least on
>>>>> any system with a /proc/sys/kernel/core_pattern passing the dump to
>>>>> another program. After this series, it does no longer.
>>>>>
>>>>>>> because qemu-io -c abort may generate core dumps
>>>>>>> even with ulimit -c 0 (and the output then contains "(core dumped)").
>>>>>> How?
>>>>> See the patches[1][2] by Mao Chuan Li. If
>>>>> /proc/sys/kernel/core_pattern passes the dump to another program,
>>>>> ulimit -c 0 does not matter.
>>>>>
>>>>> [1] http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg02092.html
>>>>> [2] http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg02093.html
>>>>>
>>>>> The problem with those patches is that they require access to
>>>>> /proc/sys/kernel/core_pattern. I don't like having to run the iotests
>>>>> as root.
>>>> To me, this sounds like a case of "doctor, it hurts when I do this".
>>> What do you mean? That I don't want the iotests to run as root? Or
>>> that I don't want to go the alternative of filtering out the "(core
>>> dumped)" message?
>> I mean:
>>
>> Doctor, it hurts when I write weird stuff to
>> /proc/sys/kernel/core_pattern.
>>
>> Don't do that then.
>>
>> If you want to be a nicer doc than me, go right ahead.
>
> I don't write weird stuff there. My default system configuration does
> (and mine is not the only one):
>
> $ uname -r
> 3.17.3-200.fc20.x86_64
> $ cat /proc/sys/kernel/core_pattern
> |/usr/sbin/chroot /proc/%P/root /usr/libexec/abrt-hook-ccpp %s %c %p
> %u %g %t e
abrt is one of the things I kill with prejudice on all my development
machines.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] iotests: Fix test 039
2014-11-25 10:09 [Qemu-devel] [PATCH 0/3] iotests: Fix test 039 Max Reitz
` (3 preceding siblings ...)
2014-11-25 12:21 ` [Qemu-devel] [PATCH 0/3] " Markus Armbruster
@ 2014-11-25 16:48 ` Michael Mueller
4 siblings, 0 replies; 17+ messages in thread
From: Michael Mueller @ 2014-11-25 16:48 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Mao Chuan Li, Fam Zheng, qemu-devel, Stefan Hajnoczi
On Tue, 25 Nov 2014 11:09:35 +0100
Max Reitz <mreitz@redhat.com> wrote:
> Test 039 used to fail because qemu-io -c abort may generate core dumps
> even with ulimit -c 0 (and the output then contains "(core dumped)").
> Fix this by adding an option to the "abort" command which allows
> specifying a signal number to raise(). Using SIGKILL for example does
> not result in a core dump, but it still badly crashes qemu-io (as
> desired).
>
> I am sending this series because we need all tests to work before adding
> the check-block target to "make check" (which we will hopefully do
> soon).
>
>
> Max Reitz (3):
> qemu-io: Let -c abort raise any signal
> iotests: Filter for "Killed" in qemu-io output
> iotests: Fix test 039
>
> qemu-io-cmds.c | 59 ++++++++++++++++++++++++++++++++++++++--
> tests/qemu-iotests/039 | 12 ++++----
> tests/qemu-iotests/039.out | 6 ++--
> tests/qemu-iotests/common.filter | 2 +-
> 4 files changed, 67 insertions(+), 12 deletions(-)
>
Hi Max,
thanks a lot for picking this up to make our poor circumvention obsolete.
Michael
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] iotests: Fix test 039
2014-11-25 14:04 ` Markus Armbruster
@ 2014-11-25 16:53 ` Michael Mueller
0 siblings, 0 replies; 17+ messages in thread
From: Michael Mueller @ 2014-11-25 16:53 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Fam Zheng, Mao Chuan Li, qemu-devel, Max Reitz,
Stefan Hajnoczi
On Tue, 25 Nov 2014 15:04:38 +0100
Markus Armbruster <armbru@redhat.com> wrote:
> Max Reitz <mreitz@redhat.com> writes:
>
> > On 2014-11-25 at 14:48, Markus Armbruster wrote:
> >> Max Reitz <mreitz@redhat.com> writes:
> >>
> >>> On 2014-11-25 at 14:20, Markus Armbruster wrote:
> >>>> Max Reitz <mreitz@redhat.com> writes:
> >>>>
> >>>>> On 2014-11-25 at 13:21, Markus Armbruster wrote:
> >>>>>> Max Reitz <mreitz@redhat.com> writes:
> >>>>>>
> >>>>>>> Test 039 used to fail
> >>>>>> I'm confused: "used to" suggests it doesn't anymore, but you sending a
> >>>>>> patches strongly suggests something's broken.
> >>>>> Well, it used to fail before this series. :-P
> >>>>>
> >>>>> You're right, this sounds bad. Currently, 039 does fail, at least on
> >>>>> any system with a /proc/sys/kernel/core_pattern passing the dump to
> >>>>> another program. After this series, it does no longer.
> >>>>>
> >>>>>>> because qemu-io -c abort may generate core dumps
> >>>>>>> even with ulimit -c 0 (and the output then contains "(core dumped)").
> >>>>>> How?
> >>>>> See the patches[1][2] by Mao Chuan Li. If
> >>>>> /proc/sys/kernel/core_pattern passes the dump to another program,
> >>>>> ulimit -c 0 does not matter.
> >>>>>
> >>>>> [1] http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg02092.html
> >>>>> [2] http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg02093.html
> >>>>>
> >>>>> The problem with those patches is that they require access to
> >>>>> /proc/sys/kernel/core_pattern. I don't like having to run the iotests
> >>>>> as root.
> >>>> To me, this sounds like a case of "doctor, it hurts when I do this".
> >>> What do you mean? That I don't want the iotests to run as root? Or
> >>> that I don't want to go the alternative of filtering out the "(core
> >>> dumped)" message?
> >> I mean:
> >>
> >> Doctor, it hurts when I write weird stuff to
> >> /proc/sys/kernel/core_pattern.
> >>
> >> Don't do that then.
> >>
> >> If you want to be a nicer doc than me, go right ahead.
> >
> > I don't write weird stuff there. My default system configuration does
> > (and mine is not the only one):
> >
> > $ uname -r
> > 3.17.3-200.fc20.x86_64
> > $ cat /proc/sys/kernel/core_pattern
> > |/usr/sbin/chroot /proc/%P/root /usr/libexec/abrt-hook-ccpp %s %c %p
> > %u %g %t e
>
> abrt is one of the things I kill with prejudice on all my development
> machines.
Markus,
we use non-personal test machines that shall automatically start tests like iotests when patches
are committed, thus this series really helps... Thanks.
Michael
>
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2014-11-25 16:54 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-11-25 10:09 [Qemu-devel] [PATCH 0/3] iotests: Fix test 039 Max Reitz
2014-11-25 10:09 ` [Qemu-devel] [PATCH 1/3] qemu-io: Let -c abort raise any signal Max Reitz
2014-11-25 13:19 ` Markus Armbruster
2014-11-25 13:31 ` Max Reitz
2014-11-25 10:09 ` [Qemu-devel] [PATCH 2/3] iotests: Filter for "Killed" in qemu-io output Max Reitz
2014-11-25 13:05 ` Markus Armbruster
2014-11-25 13:06 ` Max Reitz
2014-11-25 10:09 ` [Qemu-devel] [PATCH 3/3] iotests: Fix test 039 Max Reitz
2014-11-25 12:21 ` [Qemu-devel] [PATCH 0/3] " Markus Armbruster
2014-11-25 12:29 ` Max Reitz
2014-11-25 13:20 ` Markus Armbruster
2014-11-25 13:22 ` Max Reitz
2014-11-25 13:48 ` Markus Armbruster
2014-11-25 13:50 ` Max Reitz
2014-11-25 14:04 ` Markus Armbruster
2014-11-25 16:53 ` Michael Mueller
2014-11-25 16:48 ` Michael Mueller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).