qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [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).