[Qemu-devel] [PATCH 0/1] libqtest: Improve error reporting for bad read from QEMU

[Qemu-devel] [PATCH 0/1] libqtest: Improve error reporting for bad read from QEMU

[Markus Armbruster]

Eric Blake's "[PATCH v4] tests/libqtest: Improve kill_qemu()" improves
how we report how QEMU terminated.  Sadly, we bypass kill_qemu() in
certain failure modes.  Fix that.

Based-on: <20180810132800.38549-1-eblake@redhat.com>

Markus Armbruster (1):
  libqtest: Improve error reporting for bad read from QEMU

 tests/libqtest.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/1] libqtest: Improve error reporting for bad read from QEMU

[Markus Armbruster]

When read() from the qtest socket or the QMP socket fails or EOFs, we
report "Broken pipe" and exit(1).  This commonly happens when QEMU
crashes.  It also happens when QEMU refuses to run because the test
passed it bad arguments.  Sadly, we neglect to report either.

Improve this by calling abort() instead of exit(1), so kill_qemu()
runs, and reports how QEMU died.  This improves error reporting to
something like

    /x86_64/device/introspect/list: Broken pipe
    tests/libqtest.c:129: kill_qemu() detected QEMU death from signal 6 (Aborted) (dumped core)

Three exit() remain in libqtest.c:

* In qmp_response(), when we can't parse a QMP reply read from the QMP
  socket.  Change to abort() for consistency.

* In qtest_qemu_binary(), when QTEST_QEMU_BINARY isn't in the
  environment.  This can only happen before we start QEMU.  Leave
  alone.

* In qtest_init_without_qmp_handshake(), when the fork()ed child fails
  to execlp().  Leave alone.

exit() elsewhere are unlikely due to QEMU dying on us.  If that should
turn out to be wrong, we can move kill_qemu() from the abrt handler to
atexit() or something.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/libqtest.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index b703fca26d..852ccff1ce 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -367,7 +367,7 @@ static GString *qtest_recv_line(QTestState *s)
 
         if (len == -1 || len == 0) {
             fprintf(stderr, "Broken pipe\n");
-            exit(1);
+            abort();
         }
 
         g_string_append_len(s->rx, buffer, len);
@@ -454,7 +454,7 @@ static void qmp_response(JSONMessageParser *parser, GQueue *tokens)
     obj = json_parser_parse(tokens, NULL);
     if (!obj) {
         fprintf(stderr, "QMP JSON response parsing failed\n");
-        exit(1);
+        abort();
     }
 
     g_assert(!qmp->response);
@@ -480,7 +480,7 @@ QDict *qmp_fd_receive(int fd)
 
         if (len == -1 || len == 0) {
             fprintf(stderr, "Broken pipe\n");
-            exit(1);
+            abort();
         }
 
         if (log) {
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/1] libqtest: Improve error reporting for bad read from QEMU

[Philippe Mathieu-Daudé]

On 08/15/2018 11:19 AM, Markus Armbruster wrote:
> When read() from the qtest socket or the QMP socket fails or EOFs, we
> report "Broken pipe" and exit(1).  This commonly happens when QEMU
> crashes.  It also happens when QEMU refuses to run because the test
> passed it bad arguments.  Sadly, we neglect to report either.
> 
> Improve this by calling abort() instead of exit(1), so kill_qemu()
> runs, and reports how QEMU died.  This improves error reporting to
> something like
> 
>     /x86_64/device/introspect/list: Broken pipe
>     tests/libqtest.c:129: kill_qemu() detected QEMU death from signal 6 (Aborted) (dumped core)
> 
> Three exit() remain in libqtest.c:
> 
> * In qmp_response(), when we can't parse a QMP reply read from the QMP
>   socket.  Change to abort() for consistency.
> 
> * In qtest_qemu_binary(), when QTEST_QEMU_BINARY isn't in the
>   environment.  This can only happen before we start QEMU.  Leave
>   alone.
> 
> * In qtest_init_without_qmp_handshake(), when the fork()ed child fails
>   to execlp().  Leave alone.
> 
> exit() elsewhere are unlikely due to QEMU dying on us.  If that should
> turn out to be wrong, we can move kill_qemu() from the abrt handler to

"abort handler"?

> atexit() or something.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  tests/libqtest.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index b703fca26d..852ccff1ce 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -367,7 +367,7 @@ static GString *qtest_recv_line(QTestState *s)
>  
>          if (len == -1 || len == 0) {
>              fprintf(stderr, "Broken pipe\n");
> -            exit(1);
> +            abort();
>          }
>  
>          g_string_append_len(s->rx, buffer, len);
> @@ -454,7 +454,7 @@ static void qmp_response(JSONMessageParser *parser, GQueue *tokens)
>      obj = json_parser_parse(tokens, NULL);
>      if (!obj) {
>          fprintf(stderr, "QMP JSON response parsing failed\n");
> -        exit(1);
> +        abort();
>      }
>  
>      g_assert(!qmp->response);
> @@ -480,7 +480,7 @@ QDict *qmp_fd_receive(int fd)
>  
>          if (len == -1 || len == 0) {
>              fprintf(stderr, "Broken pipe\n");
> -            exit(1);
> +            abort();
>          }
>  
>          if (log) {
> 

Re: [Qemu-devel] [PATCH 1/1] libqtest: Improve error reporting for bad read from QEMU

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 08/15/2018 11:19 AM, Markus Armbruster wrote:
>> When read() from the qtest socket or the QMP socket fails or EOFs, we
>> report "Broken pipe" and exit(1).  This commonly happens when QEMU
>> crashes.  It also happens when QEMU refuses to run because the test
>> passed it bad arguments.  Sadly, we neglect to report either.
>> 
>> Improve this by calling abort() instead of exit(1), so kill_qemu()
>> runs, and reports how QEMU died.  This improves error reporting to
>> something like
>> 
>>     /x86_64/device/introspect/list: Broken pipe
>>     tests/libqtest.c:129: kill_qemu() detected QEMU death from signal 6 (Aborted) (dumped core)
>> 
>> Three exit() remain in libqtest.c:
>> 
>> * In qmp_response(), when we can't parse a QMP reply read from the QMP
>>   socket.  Change to abort() for consistency.
>> 
>> * In qtest_qemu_binary(), when QTEST_QEMU_BINARY isn't in the
>>   environment.  This can only happen before we start QEMU.  Leave
>>   alone.
>> 
>> * In qtest_init_without_qmp_handshake(), when the fork()ed child fails
>>   to execlp().  Leave alone.
>> 
>> exit() elsewhere are unlikely due to QEMU dying on us.  If that should
>> turn out to be wrong, we can move kill_qemu() from the abrt handler to
>
> "abort handler"?

The variable is called @abrt_hooks, like SIGABRT[*].  But you're right,
I should either use the English word spelled correctly, or use the
identifier spelled correctly.  I think I'll use the identifier.

>> atexit() or something.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thanks!


[*] The ancients sacrificed the occasional vowel to the God of Spartan
Linkers and ASR-33 Teletypes.

Re: [Qemu-devel] [PATCH 1/1] libqtest: Improve error reporting for bad read from QEMU

[Eric Blake]

On 08/15/2018 09:19 AM, Markus Armbruster wrote:
> When read() from the qtest socket or the QMP socket fails or EOFs, we
> report "Broken pipe" and exit(1).  This commonly happens when QEMU
> crashes.  It also happens when QEMU refuses to run because the test
> passed it bad arguments.  Sadly, we neglect to report either.
> 
> Improve this by calling abort() instead of exit(1), so kill_qemu()
> runs, and reports how QEMU died.  This improves error reporting to
> something like
> 
>      /x86_64/device/introspect/list: Broken pipe
>      tests/libqtest.c:129: kill_qemu() detected QEMU death from signal 6 (Aborted) (dumped core)

Matches your touchup to my v4 patch.

> 
> Three exit() remain in libqtest.c:
> 
> * In qmp_response(), when we can't parse a QMP reply read from the QMP
>    socket.  Change to abort() for consistency.
> 
> * In qtest_qemu_binary(), when QTEST_QEMU_BINARY isn't in the
>    environment.  This can only happen before we start QEMU.  Leave
>    alone.
> 
> * In qtest_init_without_qmp_handshake(), when the fork()ed child fails
>    to execlp().  Leave alone.

exit() after failed execlp() is often wrong (in particular, if you are 
fork()ed from a multi-threaded process, you REALLY want to be using 
_exit() instead).  But as you say, not the goal of this particular patch.

> 
> exit() elsewhere are unlikely due to QEMU dying on us.  If that should
> turn out to be wrong, we can move kill_qemu() from the abrt handler to
> atexit() or something.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/libqtest.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 1/1] libqtest: Improve error reporting for bad read from QEMU

[Paolo Bonzini]

On 15/08/2018 16:19, Markus Armbruster wrote:
> When read() from the qtest socket or the QMP socket fails or EOFs, we
> report "Broken pipe" and exit(1).  This commonly happens when QEMU
> crashes.  It also happens when QEMU refuses to run because the test
> passed it bad arguments.  Sadly, we neglect to report either.
> 
> Improve this by calling abort() instead of exit(1), so kill_qemu()
> runs, and reports how QEMU died.  This improves error reporting to
> something like
> 
>     /x86_64/device/introspect/list: Broken pipe
>     tests/libqtest.c:129: kill_qemu() detected QEMU death from signal 6 (Aborted) (dumped core)
> 
> Three exit() remain in libqtest.c:
> 
> * In qmp_response(), when we can't parse a QMP reply read from the QMP
>   socket.  Change to abort() for consistency.
> 
> * In qtest_qemu_binary(), when QTEST_QEMU_BINARY isn't in the
>   environment.  This can only happen before we start QEMU.  Leave
>   alone.
> 
> * In qtest_init_without_qmp_handshake(), when the fork()ed child fails
>   to execlp().  Leave alone.
> 
> exit() elsewhere are unlikely due to QEMU dying on us.  If that should
> turn out to be wrong, we can move kill_qemu() from the abrt handler to
> atexit() or something.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/libqtest.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index b703fca26d..852ccff1ce 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -367,7 +367,7 @@ static GString *qtest_recv_line(QTestState *s)
>  
>          if (len == -1 || len == 0) {
>              fprintf(stderr, "Broken pipe\n");
> -            exit(1);
> +            abort();
>          }
>  
>          g_string_append_len(s->rx, buffer, len);
> @@ -454,7 +454,7 @@ static void qmp_response(JSONMessageParser *parser, GQueue *tokens)
>      obj = json_parser_parse(tokens, NULL);
>      if (!obj) {
>          fprintf(stderr, "QMP JSON response parsing failed\n");
> -        exit(1);
> +        abort();
>      }
>  
>      g_assert(!qmp->response);
> @@ -480,7 +480,7 @@ QDict *qmp_fd_receive(int fd)
>  
>          if (len == -1 || len == 0) {
>              fprintf(stderr, "Broken pipe\n");
> -            exit(1);
> +            abort();
>          }
>  
>          if (log) {
> 

Queued, thanks.

Paolo