[Qemu-devel] [PATCH 0/3] libqtest: solve QEMU process cleanup problem

[Qemu-devel] [PATCH 0/3] libqtest: solve QEMU process cleanup problem

[Stefan Hajnoczi]

Test cases are supposed to clean up even if they fail.  Historically libqtest
has leaked QEMU processes and files.  This caused annoyances and buildbot
failures so it was gradually fixed.

The solution we have for terminating the QEMU process if the test case fails
was not very satisfactory.  A SIGABRT handler in the test case sends SIGTERM to
QEMU.  This only works if the test case receives SIGABRT, not other ways in
which it could die.  The approach is ugly because it installs a global signal
handler although libqtest is supposed to support multiple simultaneous
instances.

This patch series adds the new -chardev exit-on-eof option.  QEMU will
terminate if the socket/pipe/stdio receives EOF.  That happens when the test
case process terminates for any reason.  By adding this option to -chardev we
can use it with both -qtest and -qmp.

Stefan Hajnoczi (3):
  libqemustub: add qemu_system_shutdown_request() and no_shutdown
  qemu-char: add -chardev exit-on-eof option
  libqtest: use -chardev exit-on-eof to clean up QEMU

 include/sysemu/char.h |  1 +
 qapi-schema.json      | 23 ++++++++++++++++-------
 qemu-char.c           | 34 ++++++++++++++++++++++++++++------
 qemu-options.hx       | 19 +++++++++++++------
 stubs/Makefile.objs   |  1 +
 stubs/shutdown.c      |  7 +++++++
 tests/libqtest.c      | 48 +++---------------------------------------------
 7 files changed, 69 insertions(+), 64 deletions(-)
 create mode 100644 stubs/shutdown.c

-- 
1.9.3

[Qemu-devel] [PATCH 1/3] libqemustub: add qemu_system_shutdown_request() and no_shutdown

[Stefan Hajnoczi]

These sysemu functions will be needed by qemu-char.c, which is linked
into tests/vhost-user-test without system emulation functionality.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 stubs/Makefile.objs | 1 +
 stubs/shutdown.c    | 7 +++++++
 2 files changed, 8 insertions(+)
 create mode 100644 stubs/shutdown.c

diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 528e161..f17d517 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -39,3 +39,4 @@ stub-obj-$(CONFIG_WIN32) += fd-register.o
 stub-obj-y += cpus.o
 stub-obj-y += kvm.o
 stub-obj-y += qmp_pc_dimm_device_list.o
+stub-obj-y += shutdown.o
diff --git a/stubs/shutdown.c b/stubs/shutdown.c
new file mode 100644
index 0000000..c1aee7e
--- /dev/null
+++ b/stubs/shutdown.c
@@ -0,0 +1,7 @@
+#include "sysemu/sysemu.h"
+
+int no_shutdown;
+
+void qemu_system_shutdown_request(void)
+{
+}
-- 
1.9.3

Re: [Qemu-devel] [PATCH 1/3] libqemustub: add qemu_system_shutdown_request() and no_shutdown

[Peter Maydell]

On 24 July 2014 17:39, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> +++ b/stubs/shutdown.c
> @@ -0,0 +1,7 @@
> +#include "sysemu/sysemu.h"
> +
> +int no_shutdown;
> +
> +void qemu_system_shutdown_request(void)
> +{
> +}

Tangential, but every use of "no_shutdown" outside vl.c
is in the sequence:
                no_shutdown = 0;
                qemu_system_shutdown_request();

so maybe we should have a
   qemu_system_shutdown_demand();
and make the no_shutdown variable private to vl.c ?

(feel free to pick a better name :-))

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/3] libqemustub: add qemu_system_shutdown_request() and no_shutdown

[Stefan Hajnoczi]

On Thu, Jul 24, 2014 at 6:07 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 24 July 2014 17:39, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>> +++ b/stubs/shutdown.c
>> @@ -0,0 +1,7 @@
>> +#include "sysemu/sysemu.h"
>> +
>> +int no_shutdown;
>> +
>> +void qemu_system_shutdown_request(void)
>> +{
>> +}
>
> Tangential, but every use of "no_shutdown" outside vl.c
> is in the sequence:
>                 no_shutdown = 0;
>                 qemu_system_shutdown_request();
>
> so maybe we should have a
>    qemu_system_shutdown_demand();
> and make the no_shutdown variable private to vl.c ?
>
> (feel free to pick a better name :-))

Sure, I will find a solution for v2.

Stefan

[Qemu-devel] [PATCH 2/3] qemu-char: add -chardev exit-on-eof option

[Stefan Hajnoczi]

When QEMU is executed as part of a test case or from a script, it is
usually desirable to exit if the parent process terminates.  This
ensures that "leaked" QEMU processes do not continue consuming resources
after their parent has died.

This patch adds the -chardev exit-on-eof option causing socket and pipe
chardevs to exit QEMU upon close.  This happens when a parent process
deliberately closes its file descriptor but also when the kernel cleans
up a crashed process.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/sysemu/char.h |  1 +
 qapi-schema.json      | 23 ++++++++++++++++-------
 qemu-char.c           | 34 ++++++++++++++++++++++++++++------
 qemu-options.hx       | 19 +++++++++++++------
 4 files changed, 58 insertions(+), 19 deletions(-)

diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 0bbd631..382b320 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -86,6 +86,7 @@ struct CharDriverState {
     guint fd_in_tag;
     QemuOpts *opts;
     QTAILQ_ENTRY(CharDriverState) next;
+    bool exit_on_eof;
 };
 
 /**
diff --git a/qapi-schema.json b/qapi-schema.json
index b11aad2..9b13da1 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2630,10 +2630,13 @@
 # @device: The name of the special file for the device,
 #          i.e. /dev/ttyS0 on Unix or COM1: on Windows
 # @type: What kind of device this is.
+# @exit-on-eof: #optional terminate when other side closes the pipe
+#               (default: false, since: 2.2)
 #
 # Since: 1.4
 ##
-{ 'type': 'ChardevHostdev', 'data': { 'device' : 'str' } }
+{ 'type': 'ChardevHostdev', 'data': { 'device' : 'str',
+                                      '*exit-on-eof' : 'bool' } }
 
 ##
 # @ChardevSocket:
@@ -2648,14 +2651,17 @@
 # @nodelay: #optional set TCP_NODELAY socket option (default: false)
 # @telnet: #optional enable telnet protocol on server
 #          sockets (default: false)
+# @exit-on-eof: #optional terminate when other side closes socket
+#               (default: false, since: 2.2)
 #
 # Since: 1.4
 ##
-{ 'type': 'ChardevSocket', 'data': { 'addr'     : 'SocketAddress',
-                                     '*server'  : 'bool',
-                                     '*wait'    : 'bool',
-                                     '*nodelay' : 'bool',
-                                     '*telnet'  : 'bool' } }
+{ 'type': 'ChardevSocket', 'data': { 'addr'         : 'SocketAddress',
+                                     '*server'      : 'bool',
+                                     '*wait'        : 'bool',
+                                     '*nodelay'     : 'bool',
+                                     '*telnet'      : 'bool',
+                                     '*exit-on-eof' : 'bool' } }
 
 ##
 # @ChardevUdp:
@@ -2689,10 +2695,13 @@
 # @signal: #optional Allow signals (such as SIGINT triggered by ^C)
 #          be delivered to qemu.  Default: true in -nographic mode,
 #          false otherwise.
+# @exit-on-eof: #optional terminate when other side sends EOF
+#               (default: false, since: 2.2)
 #
 # Since: 1.5
 ##
-{ 'type': 'ChardevStdio', 'data': { '*signal' : 'bool' } }
+{ 'type': 'ChardevStdio', 'data': { '*signal' : 'bool',
+                                    '*exit-on-eof' : 'bool' } }
 
 ##
 # @ChardevSpiceChannel:
diff --git a/qemu-char.c b/qemu-char.c
index 7acc03f..9015bc9 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -110,9 +110,16 @@ void qemu_chr_be_event(CharDriverState *s, int event)
             break;
     }
 
-    if (!s->chr_event)
-        return;
-    s->chr_event(s->handler_opaque, event);
+    if (s->chr_event) {
+        s->chr_event(s->handler_opaque, event);
+    }
+
+    if (s->exit_on_eof && event == CHR_EVENT_CLOSED) {
+        fprintf(stderr, "qemu: terminating due to eof on chardev '%s'\n",
+                s->label);
+        no_shutdown = 0;
+        qemu_system_shutdown_request();
+    }
 }
 
 void qemu_chr_be_generic_open(CharDriverState *s)
@@ -991,6 +998,7 @@ static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts)
     int fd_in, fd_out;
     char filename_in[256], filename_out[256];
     const char *filename = opts->device;
+    CharDriverState *chr;
 
     if (filename == NULL) {
         fprintf(stderr, "chardev: pipe: no filename given\n");
@@ -1011,7 +1019,9 @@ static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts)
             return NULL;
         }
     }
-    return qemu_chr_open_fd(fd_in, fd_out);
+    chr = qemu_chr_open_fd(fd_in, fd_out);
+    chr->exit_on_eof = opts->has_exit_on_eof && opts->exit_on_eof;
+    return chr;
 }
 
 /* init terminal so that we can grab keys */
@@ -2893,6 +2903,7 @@ static void tcp_chr_close(CharDriverState *chr)
 static CharDriverState *qemu_chr_open_socket_fd(int fd, bool do_nodelay,
                                                 bool is_listen, bool is_telnet,
                                                 bool is_waitconnect,
+                                                bool is_exit_on_eof,
                                                 Error **errp)
 {
     CharDriverState *chr = NULL;
@@ -2955,6 +2966,7 @@ static CharDriverState *qemu_chr_open_socket_fd(int fd, bool do_nodelay,
     chr->chr_update_read_handler = tcp_chr_update_read_handler;
     /* be isn't opened until we get a connection */
     chr->explicit_be_open = true;
+    chr->exit_on_eof = is_exit_on_eof;
 
     if (is_listen) {
         s->listen_fd = fd;
@@ -2991,6 +3003,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
     bool is_telnet      = qemu_opt_get_bool(opts, "telnet", false);
     bool do_nodelay     = !qemu_opt_get_bool(opts, "delay", true);
     bool is_unix        = qemu_opt_get(opts, "path") != NULL;
+    bool is_exit_on_eof = qemu_opt_get_bool(opts, "exit-on-eof", false);
 
     if (is_unix) {
         if (is_listen) {
@@ -3013,7 +3026,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
         qemu_set_nonblock(fd);
 
     chr = qemu_chr_open_socket_fd(fd, do_nodelay, is_listen, is_telnet,
-                                  is_waitconnect, &local_err);
+                                  is_waitconnect, is_exit_on_eof, &local_err);
     if (local_err) {
         goto fail;
     }
@@ -3376,6 +3389,8 @@ static void qemu_chr_parse_stdio(QemuOpts *opts, ChardevBackend *backend,
     backend->stdio = g_new0(ChardevStdio, 1);
     backend->stdio->has_signal = true;
     backend->stdio->signal = qemu_opt_get_bool(opts, "signal", true);
+    backend->stdio->has_exit_on_eof = true;
+    backend->stdio->exit_on_eof = qemu_opt_get_bool(opts, "exit-on-eof", false);
 }
 
 static void qemu_chr_parse_serial(QemuOpts *opts, ChardevBackend *backend,
@@ -3415,6 +3430,8 @@ static void qemu_chr_parse_pipe(QemuOpts *opts, ChardevBackend *backend,
     }
     backend->pipe = g_new0(ChardevHostdev, 1);
     backend->pipe->device = g_strdup(device);
+    backend->pipe->has_exit_on_eof = true;
+    backend->pipe->exit_on_eof = qemu_opt_get_bool(opts, "exit-on-eof", false);
 }
 
 static void qemu_chr_parse_ringbuf(QemuOpts *opts, ChardevBackend *backend,
@@ -3848,6 +3865,9 @@ QemuOptsList qemu_chardev_opts = {
         },{
             .name = "chardev",
             .type = QEMU_OPT_STRING,
+        },{
+            .name = "exit-on-eof",
+            .type = QEMU_OPT_BOOL,
         },
         { /* end of list */ }
     },
@@ -3967,6 +3987,7 @@ static CharDriverState *qmp_chardev_open_socket(ChardevSocket *sock,
     bool is_listen      = sock->has_server  ? sock->server  : true;
     bool is_telnet      = sock->has_telnet  ? sock->telnet  : false;
     bool is_waitconnect = sock->has_wait    ? sock->wait    : false;
+    bool is_exit_on_eof = sock->has_exit_on_eof ? sock->exit_on_eof : false;
     int fd;
 
     if (is_listen) {
@@ -3978,7 +3999,8 @@ static CharDriverState *qmp_chardev_open_socket(ChardevSocket *sock,
         return NULL;
     }
     return qemu_chr_open_socket_fd(fd, do_nodelay, is_listen,
-                                   is_telnet, is_waitconnect, errp);
+                                   is_telnet, is_waitconnect,
+                                   is_exit_on_eof, errp);
 }
 
 static CharDriverState *qmp_chardev_open_udp(ChardevUdp *udp,
diff --git a/qemu-options.hx b/qemu-options.hx
index 9e54686..4b4da4f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1927,8 +1927,9 @@ ETEXI
 DEF("chardev", HAS_ARG, QEMU_OPTION_chardev,
     "-chardev null,id=id[,mux=on|off]\n"
     "-chardev socket,id=id[,host=host],port=host[,to=to][,ipv4][,ipv6][,nodelay]\n"
-    "         [,server][,nowait][,telnet][,mux=on|off] (tcp)\n"
-    "-chardev socket,id=id,path=path[,server][,nowait][,telnet],[mux=on|off] (unix)\n"
+    "         [,server][,nowait][,telnet][,mux=on|off][,exit-on-eof] (tcp)\n"
+    "-chardev socket,id=id,path=path[,server][,nowait][,telnet],[mux=on|off]\n"
+    "         [,exit-on-eof] (unix)\n"
     "-chardev udp,id=id[,host=host],port=port[,localaddr=localaddr]\n"
     "         [,localport=localport][,ipv4][,ipv6][,mux=on|off]\n"
     "-chardev msmouse,id=id[,mux=on|off]\n"
@@ -1936,13 +1937,13 @@ DEF("chardev", HAS_ARG, QEMU_OPTION_chardev,
     "         [,mux=on|off]\n"
     "-chardev ringbuf,id=id[,size=size]\n"
     "-chardev file,id=id,path=path[,mux=on|off]\n"
-    "-chardev pipe,id=id,path=path[,mux=on|off]\n"
+    "-chardev pipe,id=id,path=path[,mux=on|off][,exit-on-eof]\n"
 #ifdef _WIN32
     "-chardev console,id=id[,mux=on|off]\n"
     "-chardev serial,id=id,path=path[,mux=on|off]\n"
 #else
     "-chardev pty,id=id[,mux=on|off]\n"
-    "-chardev stdio,id=id[,mux=on|off][,signal=on|off]\n"
+    "-chardev stdio,id=id[,mux=on|off][,signal=on|off][,exit-on-eof]\n"
 #endif
 #ifdef CONFIG_BRLAPI
     "-chardev braille,id=id[,mux=on|off]\n"
@@ -2000,7 +2001,7 @@ Options to each backend are described below.
 A void device. This device will not emit any data, and will drop any data it
 receives. The null backend does not take any options.
 
-@item -chardev socket ,id=@var{id} [@var{TCP options} or @var{unix options}] [,server] [,nowait] [,telnet]
+@item -chardev socket ,id=@var{id} [@var{TCP options} or @var{unix options}] [,server] [,nowait] [,telnet] [,exit-on-eof]
 
 Create a two-way stream socket, which can be either a TCP or a unix socket. A
 unix socket will be created if @option{path} is specified. Behaviour is
@@ -2014,6 +2015,8 @@ connect to a listening socket.
 @option{telnet} specifies that traffic on the socket should interpret telnet
 escape sequences.
 
+@option{exit-on-eof} specifies that QEMU should terminate upon disconnect
+
 TCP and unix socket options are given below:
 
 @table @option
@@ -2094,7 +2097,7 @@ Log all traffic received from the guest to a file.
 created if it does not already exist, and overwritten if it does. @option{path}
 is required.
 
-@item -chardev pipe ,id=@var{id} ,path=@var{path}
+@item -chardev pipe ,id=@var{id} ,path=@var{path} [,exit-on-eof]
 
 Create a two-way connection to the guest. The behaviour differs slightly between
 Windows hosts and other hosts:
@@ -2111,6 +2114,8 @@ be present.
 @option{path} forms part of the pipe path as described above. @option{path} is
 required.
 
+@option{exit-on-eof} specifies that QEMU should terminate upon disconnect
+
 @item -chardev console ,id=@var{id}
 
 Send traffic from the guest to QEMU's standard output. @option{console} does not
@@ -2141,6 +2146,8 @@ Connect to standard input and standard output of the QEMU process.
 exiting QEMU with the key sequence @key{Control-c}. This option is enabled by
 default, use @option{signal=off} to disable it.
 
+@option{exit-on-eof} specifies that QEMU should terminate upon EOF
+
 @option{stdio} is not available on Windows hosts.
 
 @item -chardev braille ,id=@var{id}
-- 
1.9.3

Re: [Qemu-devel] [PATCH 2/3] qemu-char: add -chardev exit-on-eof option

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> When QEMU is executed as part of a test case or from a script, it is
> usually desirable to exit if the parent process terminates.  This
> ensures that "leaked" QEMU processes do not continue consuming resources
> after their parent has died.
>
> This patch adds the -chardev exit-on-eof option causing socket and pipe
> chardevs to exit QEMU upon close.  This happens when a parent process
> deliberately closes its file descriptor but also when the kernel cleans
> up a crashed process.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/sysemu/char.h |  1 +
>  qapi-schema.json      | 23 ++++++++++++++++-------
>  qemu-char.c           | 34 ++++++++++++++++++++++++++++------
>  qemu-options.hx       | 19 +++++++++++++------
>  4 files changed, 58 insertions(+), 19 deletions(-)
>
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index 0bbd631..382b320 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -86,6 +86,7 @@ struct CharDriverState {
>      guint fd_in_tag;
>      QemuOpts *opts;
>      QTAILQ_ENTRY(CharDriverState) next;
> +    bool exit_on_eof;
>  };
>  
>  /**
> diff --git a/qapi-schema.json b/qapi-schema.json
> index b11aad2..9b13da1 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2630,10 +2630,13 @@
>  # @device: The name of the special file for the device,
>  #          i.e. /dev/ttyS0 on Unix or COM1: on Windows
>  # @type: What kind of device this is.
> +# @exit-on-eof: #optional terminate when other side closes the pipe
> +#               (default: false, since: 2.2)
>  #
>  # Since: 1.4
>  ##
> -{ 'type': 'ChardevHostdev', 'data': { 'device' : 'str' } }
> +{ 'type': 'ChardevHostdev', 'data': { 'device' : 'str',
> +                                      '*exit-on-eof' : 'bool' } }
>  
>  ##
>  # @ChardevSocket:

Any use cases beyond libqtest?

If no, should this be x-exit-on-eof?

Hmm, looks like there's no precedence for x- in QAPI.

Re: [Qemu-devel] [PATCH 2/3] qemu-char: add -chardev exit-on-eof option

[Stefan Hajnoczi]

On Fri, Jul 25, 2014 at 9:12 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
>
>> When QEMU is executed as part of a test case or from a script, it is
>> usually desirable to exit if the parent process terminates.  This
>> ensures that "leaked" QEMU processes do not continue consuming resources
>> after their parent has died.
>>
>> This patch adds the -chardev exit-on-eof option causing socket and pipe
>> chardevs to exit QEMU upon close.  This happens when a parent process
>> deliberately closes its file descriptor but also when the kernel cleans
>> up a crashed process.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  include/sysemu/char.h |  1 +
>>  qapi-schema.json      | 23 ++++++++++++++++-------
>>  qemu-char.c           | 34 ++++++++++++++++++++++++++++------
>>  qemu-options.hx       | 19 +++++++++++++------
>>  4 files changed, 58 insertions(+), 19 deletions(-)
>>
>> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
>> index 0bbd631..382b320 100644
>> --- a/include/sysemu/char.h
>> +++ b/include/sysemu/char.h
>> @@ -86,6 +86,7 @@ struct CharDriverState {
>>      guint fd_in_tag;
>>      QemuOpts *opts;
>>      QTAILQ_ENTRY(CharDriverState) next;
>> +    bool exit_on_eof;
>>  };
>>
>>  /**
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index b11aad2..9b13da1 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -2630,10 +2630,13 @@
>>  # @device: The name of the special file for the device,
>>  #          i.e. /dev/ttyS0 on Unix or COM1: on Windows
>>  # @type: What kind of device this is.
>> +# @exit-on-eof: #optional terminate when other side closes the pipe
>> +#               (default: false, since: 2.2)
>>  #
>>  # Since: 1.4
>>  ##
>> -{ 'type': 'ChardevHostdev', 'data': { 'device' : 'str' } }
>> +{ 'type': 'ChardevHostdev', 'data': { 'device' : 'str',
>> +                                      '*exit-on-eof' : 'bool' } }
>>
>>  ##
>>  # @ChardevSocket:
>
> Any use cases beyond libqtest?

qemu-iotests should use it too.  Basically any script that drives QEMU
directly will need to clean up the QEMU process.

Stefan

Re: [Qemu-devel] [PATCH 2/3] qemu-char: add -chardev exit-on-eof option

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2272 bytes --]

On 07/25/2014 02:12 AM, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
>> When QEMU is executed as part of a test case or from a script, it is
>> usually desirable to exit if the parent process terminates.  This
>> ensures that "leaked" QEMU processes do not continue consuming resources
>> after their parent has died.

>>  ##
>> -{ 'type': 'ChardevHostdev', 'data': { 'device' : 'str' } }
>> +{ 'type': 'ChardevHostdev', 'data': { 'device' : 'str',
>> +                                      '*exit-on-eof' : 'bool' } }
>>  
>>  ##
>>  # @ChardevSocket:
> 
> Any use cases beyond libqtest?

Libvirt probably won't use it for normal guests (we don't want to kill
qemu just because the monitor disconnects), but does have the notion of
an autodestroy guest where it might be useful (we WANT the guest to go
away if libvirtd dies early).  In fact, autodestroy guests are used
during migration - we want to kill qemu on the destination side if
libvirtd dies before the source side finishes sending the migration
stream.  But in that scenario, once migration succeeds, libvirt has to
be able to convert an autodestroy guest back into a normal guest that no
longer disappears when libvirtd does; would this be something that QMP
can toggle the state of this attribute on the fly?  Perhaps through qom-set?

> 
> If no, should this be x-exit-on-eof?
> 
> Hmm, looks like there's no precedence for x- in QAPI.

Ah, but we do.  For example, x-rdma-pin-all in MigrationCapability in
qemu 1.7; which has later been removed.

However, we also have precedence of actions in QAPI that are very
unlikely to be used outside of qtest, but which are not marked
experimental; for example, the 'Abort' action in 'transaction' will
probably never be used by libvirt (but as I type that, the thought in
the back of my mind is that I could possibly do some sort of feature
probing by setting up a transaction that will abort, and distinguish
between whether the transaction aborted or whether it errored out
because the feature I'm probing for isn't present).

I'm fine leaving this as plain 'exit-on-eof'.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH 2/3] qemu-char: add -chardev exit-on-eof option

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/25/2014 02:12 AM, Markus Armbruster wrote:
>> Stefan Hajnoczi <stefanha@redhat.com> writes:
>> 
>>> When QEMU is executed as part of a test case or from a script, it is
>>> usually desirable to exit if the parent process terminates.  This
>>> ensures that "leaked" QEMU processes do not continue consuming resources
>>> after their parent has died.
>
>>>  ##
>>> -{ 'type': 'ChardevHostdev', 'data': { 'device' : 'str' } }
>>> +{ 'type': 'ChardevHostdev', 'data': { 'device' : 'str',
>>> +                                      '*exit-on-eof' : 'bool' } }
>>>  
>>>  ##
>>>  # @ChardevSocket:
>> 
>> Any use cases beyond libqtest?
>
> Libvirt probably won't use it for normal guests (we don't want to kill
> qemu just because the monitor disconnects), but does have the notion of
> an autodestroy guest where it might be useful (we WANT the guest to go
> away if libvirtd dies early).  In fact, autodestroy guests are used
> during migration - we want to kill qemu on the destination side if
> libvirtd dies before the source side finishes sending the migration
> stream.  But in that scenario, once migration succeeds, libvirt has to
> be able to convert an autodestroy guest back into a normal guest that no
> longer disappears when libvirtd does; would this be something that QMP
> can toggle the state of this attribute on the fly?  Perhaps through qom-set?

After migration completes, execution moves from source to target.
Wouldn't you want to switch off target auto-destruct together with that
move, atomically?

>> If no, should this be x-exit-on-eof?
>> 
>> Hmm, looks like there's no precedence for x- in QAPI.
>
> Ah, but we do.  For example, x-rdma-pin-all in MigrationCapability in
> qemu 1.7; which has later been removed.
>
> However, we also have precedence of actions in QAPI that are very
> unlikely to be used outside of qtest, but which are not marked
> experimental; for example, the 'Abort' action in 'transaction' will
> probably never be used by libvirt

Arguably not a conscious decision to make it ABI forever, more a case of
nobody thought about *not* making it ABI :)

>                                   (but as I type that, the thought in
> the back of my mind is that I could possibly do some sort of feature
> probing by setting up a transaction that will abort, and distinguish
> between whether the transaction aborted or whether it errored out
> because the feature I'm probing for isn't present).
>
> I'm fine leaving this as plain 'exit-on-eof'.

I don't really mind this instance, but I'm a bit concerned about rank
ABI growth.

Re: [Qemu-devel] [PATCH 2/3] qemu-char: add -chardev exit-on-eof option

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2294 bytes --]

On 07/29/2014 12:12 AM, Markus Armbruster wrote:

>> Libvirt probably won't use it for normal guests (we don't want to kill
>> qemu just because the monitor disconnects), but does have the notion of
>> an autodestroy guest where it might be useful (we WANT the guest to go
>> away if libvirtd dies early).  In fact, autodestroy guests are used
>> during migration - we want to kill qemu on the destination side if
>> libvirtd dies before the source side finishes sending the migration
>> stream.  But in that scenario, once migration succeeds, libvirt has to
>> be able to convert an autodestroy guest back into a normal guest that no
>> longer disappears when libvirtd does; would this be something that QMP
>> can toggle the state of this attribute on the fly?  Perhaps through qom-set?
> 
> After migration completes, execution moves from source to target.
> Wouldn't you want to switch off target auto-destruct together with that
> move, atomically?

Libvirt starts the destination with -S, and migration can't complete
until libvirt resumes the destination CPUs with 'cont'.  Libvirt's
current timing of releasing auto-destruct is based on handshaking
between source and destination; it occurs after source claims migration
is done but before resuming CPUs on the destination, which satisfies the
atomicity that you correctly observed to be necessary.

>> However, we also have precedence of actions in QAPI that are very
>> unlikely to be used outside of qtest, but which are not marked
>> experimental; for example, the 'Abort' action in 'transaction' will
>> probably never be used by libvirt
> 
> Arguably not a conscious decision to make it ABI forever, more a case of
> nobody thought about *not* making it ABI :)

Added in June 2013; and we *did* have a discussion on whether to hide
the transaction name at the time...
https://lists.gnu.org/archive/html/qemu-devel/2013-06/msg03281.html

> I don't really mind this instance, but I'm a bit concerned about rank
> ABI growth.

And that's a good position to maintain - it's always good to justify new
knobs, especially since once they are ABI, it's harder to refactor
around them.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH 2/3] qemu-char: add -chardev exit-on-eof option

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/29/2014 12:12 AM, Markus Armbruster wrote:
>
>>> Libvirt probably won't use it for normal guests (we don't want to kill
>>> qemu just because the monitor disconnects), but does have the notion of
>>> an autodestroy guest where it might be useful (we WANT the guest to go
>>> away if libvirtd dies early).  In fact, autodestroy guests are used
>>> during migration - we want to kill qemu on the destination side if
>>> libvirtd dies before the source side finishes sending the migration
>>> stream.  But in that scenario, once migration succeeds, libvirt has to
>>> be able to convert an autodestroy guest back into a normal guest that no
>>> longer disappears when libvirtd does; would this be something that QMP
>>> can toggle the state of this attribute on the fly?  Perhaps through qom-set?
>> 
>> After migration completes, execution moves from source to target.
>> Wouldn't you want to switch off target auto-destruct together with that
>> move, atomically?
>
> Libvirt starts the destination with -S, and migration can't complete
> until libvirt resumes the destination CPUs with 'cont'.  Libvirt's
> current timing of releasing auto-destruct is based on handshaking
> between source and destination; it occurs after source claims migration
> is done but before resuming CPUs on the destination, which satisfies the
> atomicity that you correctly observed to be necessary.

Makes sense, thanks.

>>> However, we also have precedence of actions in QAPI that are very
>>> unlikely to be used outside of qtest, but which are not marked
>>> experimental; for example, the 'Abort' action in 'transaction' will
>>> probably never be used by libvirt
>> 
>> Arguably not a conscious decision to make it ABI forever, more a case of
>> nobody thought about *not* making it ABI :)
>
> Added in June 2013; and we *did* have a discussion on whether to hide
> the transaction name at the time...
> https://lists.gnu.org/archive/html/qemu-devel/2013-06/msg03281.html

I stand corrected.

>> I don't really mind this instance, but I'm a bit concerned about rank
>> ABI growth.
>
> And that's a good position to maintain - it's always good to justify new
> knobs, especially since once they are ABI, it's harder to refactor
> around them.

Exactly.

[Qemu-devel] [PATCH 3/3] libqtest: use -chardev exit-on-eof to clean up QEMU

[Stefan Hajnoczi]

When the test case aborts it is important to terminate the QEMU process
so it does not leak.  This was implemented using a SIGABRT handler
function in libqtest that sent SIGTERM to QEMU.

The SIGABRT approach is messy because it requires a global signal
handler but libqtest should support multiple simultaneous instances.

Simplify the code using the new -chardev exit-on-eof option.  QEMU will
automatically exit when our qtest socket closes.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/libqtest.c | 48 +++---------------------------------------------
 1 file changed, 3 insertions(+), 45 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 98e8f4b..6c3dd27 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -46,12 +46,8 @@ struct QTestState
     bool irq_level[MAX_IRQ];
     GString *rx;
     pid_t qemu_pid;  /* our child QEMU process */
-    struct sigaction sigact_old; /* restored on exit */
 };
 
-static GList *qtest_instances;
-static struct sigaction sigact_old;
-
 #define g_assert_no_errno(ret) do { \
     g_assert_cmpint(ret, !=, -1); \
 } while (0)
@@ -110,32 +106,6 @@ static void kill_qemu(QTestState *s)
     }
 }
 
-static void sigabrt_handler(int signo)
-{
-    GList *elem;
-    for (elem = qtest_instances; elem; elem = elem->next) {
-        kill_qemu(elem->data);
-    }
-}
-
-static void setup_sigabrt_handler(void)
-{
-    struct sigaction sigact;
-
-    /* Catch SIGABRT to clean up on g_assert() failure */
-    sigact = (struct sigaction){
-        .sa_handler = sigabrt_handler,
-        .sa_flags = SA_RESETHAND,
-    };
-    sigemptyset(&sigact.sa_mask);
-    sigaction(SIGABRT, &sigact, &sigact_old);
-}
-
-static void cleanup_sigabrt_handler(void)
-{
-    sigaction(SIGABRT, &sigact_old, NULL);
-}
-
 QTestState *qtest_init(const char *extra_args)
 {
     QTestState *s;
@@ -156,17 +126,12 @@ QTestState *qtest_init(const char *extra_args)
     sock = init_socket(socket_path);
     qmpsock = init_socket(qmp_socket_path);
 
-    /* Only install SIGABRT handler once */
-    if (!qtest_instances) {
-        setup_sigabrt_handler();
-    }
-
-    qtest_instances = g_list_prepend(qtest_instances, s);
-
     s->qemu_pid = fork();
     if (s->qemu_pid == 0) {
         command = g_strdup_printf("exec %s "
-                                  "-qtest unix:%s,nowait "
+                                  "-chardev socket,id=qtestdev,path=%s,nowait,"
+                                  "exit-on-eof "
+                                  "-qtest chardev:qtestdev "
                                   "-qtest-log /dev/null "
                                   "-qmp unix:%s,nowait "
                                   "-machine accel=qtest "
@@ -207,13 +172,6 @@ QTestState *qtest_init(const char *extra_args)
 
 void qtest_quit(QTestState *s)
 {
-    /* Uninstall SIGABRT handler on last instance */
-    if (qtest_instances && !qtest_instances->next) {
-        cleanup_sigabrt_handler();
-    }
-
-    qtest_instances = g_list_remove(qtest_instances, s);
-
     kill_qemu(s);
     close(s->fd);
     close(s->qmp_fd);
-- 
1.9.3