[Qemu-devel] [PATCH 0/1] add exit-script option to qemu

[Qemu-devel] [PATCH 0/1] add exit-script option to qemu

[Dominik Csapak]

this patch aims to execute a script when qemu exits
so that one can do cleanups when using --daemonize without
having to use the qmp monitor

for now i have mostly copied the script execution code from
the launch_script function of net/tap.c as i am not sure
if it would make sense to refactor that and if it does, where
to put such a function

Dominik Csapak (1):
  vl.c: call optional script when exiting

 qemu-options.hx | 18 ++++++++++++++++++
 vl.c            | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)

-- 
2.11.0

[Qemu-devel] [PATCH 1/1] vl.c: call optional script when exiting

[Dominik Csapak]

some users might want to call a script when qemu exits, without listening
to a qmp monitor for events when running with --daemonize

this can be used for things like external cleanups

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 qemu-options.hx | 18 ++++++++++++++++++
 vl.c            | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/qemu-options.hx b/qemu-options.hx
index f139459e80..03dcccee7f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3421,6 +3421,24 @@ This allows for instance switching to monitor to commit changes to the
 disk image.
 ETEXI
 
+DEF("exit-script", HAS_ARG, QEMU_OPTION_exit_script, \
+    "-exit-script    <file>\n"
+    "                      Execute the script in file on exit.\n"
+    "                      The arguments are:\n"
+    "                       - the shutdown reason,\n"
+    "                       - if it was really a reset (if -no-reboot was set)\n"
+    "                       - the name of the guest",
+    QEMU_ARCH_ALL)
+STEXI
+@item -exit-script @var{file}
+@findex -exit-script
+Execute the script $var{file} on exit.
+The arguments are:
+ - the shutdown reason
+ - if it was really a reset (if -no-reboot was set)
+ - the name of the guest
+ETEXI
+
 DEF("loadvm", HAS_ARG, QEMU_OPTION_loadvm, \
     "-loadvm [tag|id]\n" \
     "                start right away with a saved state (loadvm in monitor)\n",
diff --git a/vl.c b/vl.c
index cc55fe04a2..818381cd76 100644
--- a/vl.c
+++ b/vl.c
@@ -1522,6 +1522,9 @@ void vm_state_notify(int running, RunState state)
     }
 }
 
+static ShutdownCause shutdown_reason;
+static bool shutdown_was_reset;
+
 static ShutdownCause reset_requested;
 static ShutdownCause shutdown_requested;
 static int shutdown_signal;
@@ -1681,6 +1684,7 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
 void qemu_system_reset_request(ShutdownCause reason)
 {
     if (no_reboot && reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) {
+        shutdown_was_reset = true;
         shutdown_requested = reason;
     } else {
         reset_requested = reason;
@@ -1811,6 +1815,7 @@ static bool main_loop_should_exit(void)
         if (no_shutdown) {
             vm_stop(RUN_STATE_SHUTDOWN);
         } else {
+            shutdown_reason = request;
             return true;
         }
     }
@@ -2899,6 +2904,7 @@ int main(int argc, char **argv, char **envp)
     Error *err = NULL;
     bool list_data_dirs = false;
     char *dir, **dirs;
+    const char *exit_script = NULL;
     typedef struct BlockdevOptions_queue {
         BlockdevOptions *bdo;
         Location loc;
@@ -3587,6 +3593,9 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_no_shutdown:
                 no_shutdown = 1;
                 break;
+            case QEMU_OPTION_exit_script:
+                exit_script = optarg;
+                break;
             case QEMU_OPTION_show_cursor:
                 cursor_hide = 0;
                 break;
@@ -4577,5 +4586,41 @@ int main(int argc, char **argv, char **envp)
     migration_object_finalize();
     /* TODO: unref root container, check all devices are ok */
 
+    if (exit_script) {
+        int pid, status;
+        char *args[5];
+
+        /* try to launch network script */
+        pid = fork();
+        if (pid < 0) {
+            error_report("could not launch exit script '%s'", exit_script);
+            exit(1);
+        }
+        if (pid == 0) {
+            int open_max = sysconf(_SC_OPEN_MAX), i;
+
+            for (i = 3; i < open_max; i++) {
+                close(i);
+            }
+            args[0] = (char *)exit_script;
+            args[1] = g_strdup_printf("%d", shutdown_reason);
+            args[2] = g_strdup_printf("%d", shutdown_was_reset);
+            args[3] = qemu_get_vm_name();
+            args[4] = NULL;
+            execv(exit_script, args);
+            _exit(1);
+        } else {
+            while (waitpid(pid, &status, 0) != pid) {
+                /* loop */
+            }
+
+            if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) {
+                error_report("exit script '%s' failed with status %d",
+                             exit_script, status);
+                exit(1);
+            }
+        }
+    }
+
     return 0;
 }
-- 
2.11.0

Re: [Qemu-devel] [PATCH 1/1] vl.c: call optional script when exiting

[Philippe Mathieu-Daudé]

On 03/10/2018 11:13, Dominik Csapak wrote:
> some users might want to call a script when qemu exits, without listening
> to a qmp monitor for events when running with --daemonize
> 
> this can be used for things like external cleanups
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  qemu-options.hx | 18 ++++++++++++++++++
>  vl.c            | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index f139459e80..03dcccee7f 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3421,6 +3421,24 @@ This allows for instance switching to monitor to commit changes to the
>  disk image.
>  ETEXI
>  
> +DEF("exit-script", HAS_ARG, QEMU_OPTION_exit_script, \
> +    "-exit-script    <file>\n"
> +    "                      Execute the script in file on exit.\n"
> +    "                      The arguments are:\n"
> +    "                       - the shutdown reason,\n"
> +    "                       - if it was really a reset (if -no-reboot was set)\n"
> +    "                       - the name of the guest",
> +    QEMU_ARCH_ALL)
> +STEXI
> +@item -exit-script @var{file}
> +@findex -exit-script
> +Execute the script $var{file} on exit.
> +The arguments are:
> + - the shutdown reason
> + - if it was really a reset (if -no-reboot was set)
> + - the name of the guest
> +ETEXI
> +
>  DEF("loadvm", HAS_ARG, QEMU_OPTION_loadvm, \
>      "-loadvm [tag|id]\n" \
>      "                start right away with a saved state (loadvm in monitor)\n",
> diff --git a/vl.c b/vl.c
> index cc55fe04a2..818381cd76 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1522,6 +1522,9 @@ void vm_state_notify(int running, RunState state)
>      }
>  }
>  
> +static ShutdownCause shutdown_reason;
> +static bool shutdown_was_reset;
> +
>  static ShutdownCause reset_requested;
>  static ShutdownCause shutdown_requested;
>  static int shutdown_signal;
> @@ -1681,6 +1684,7 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
>  void qemu_system_reset_request(ShutdownCause reason)
>  {
>      if (no_reboot && reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) {
> +        shutdown_was_reset = true;
>          shutdown_requested = reason;
>      } else {
>          reset_requested = reason;
> @@ -1811,6 +1815,7 @@ static bool main_loop_should_exit(void)
>          if (no_shutdown) {
>              vm_stop(RUN_STATE_SHUTDOWN);
>          } else {
> +            shutdown_reason = request;
>              return true;
>          }
>      }
> @@ -2899,6 +2904,7 @@ int main(int argc, char **argv, char **envp)
>      Error *err = NULL;
>      bool list_data_dirs = false;
>      char *dir, **dirs;
> +    const char *exit_script = NULL;
>      typedef struct BlockdevOptions_queue {
>          BlockdevOptions *bdo;
>          Location loc;
> @@ -3587,6 +3593,9 @@ int main(int argc, char **argv, char **envp)
>              case QEMU_OPTION_no_shutdown:
>                  no_shutdown = 1;
>                  break;
> +            case QEMU_OPTION_exit_script:
> +                exit_script = optarg;
> +                break;
>              case QEMU_OPTION_show_cursor:
>                  cursor_hide = 0;
>                  break;
> @@ -4577,5 +4586,41 @@ int main(int argc, char **argv, char **envp)
>      migration_object_finalize();
>      /* TODO: unref root container, check all devices are ok */
>  
> +    if (exit_script) {
> +        int pid, status;
> +        char *args[5];
> +
> +        /* try to launch network script */

Not "network" anymore ;)

> +        pid = fork();
> +        if (pid < 0) {
> +            error_report("could not launch exit script '%s'", exit_script);
> +            exit(1);
> +        }
> +        if (pid == 0) {
> +            int open_max = sysconf(_SC_OPEN_MAX), i;
> +
> +            for (i = 3; i < open_max; i++) {
> +                close(i);
> +            }
> +            args[0] = (char *)exit_script;
> +            args[1] = g_strdup_printf("%d", shutdown_reason);
> +            args[2] = g_strdup_printf("%d", shutdown_was_reset);
> +            args[3] = qemu_get_vm_name();
> +            args[4] = NULL;
> +            execv(exit_script, args);
> +            _exit(1);
> +        } else {
> +            while (waitpid(pid, &status, 0) != pid) {
> +                /* loop */
> +            }
> +
> +            if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) {
> +                error_report("exit script '%s' failed with status %d",
> +                             exit_script, status);
> +                exit(1);
> +            }
> +        }
> +    }
> +
>      return 0;
>  }
> 

Re: [Qemu-devel] [PATCH 1/1] vl.c: call optional script when exiting

[Thomas Huth]

On 2018-10-03 11:13, Dominik Csapak wrote:
> some users might want to call a script when qemu exits, without listening
> to a qmp monitor for events when running with --daemonize
> 
> this can be used for things like external cleanups
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  qemu-options.hx | 18 ++++++++++++++++++
>  vl.c            | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 63 insertions(+)
[...]
> @@ -4577,5 +4586,41 @@ int main(int argc, char **argv, char **envp)
>      migration_object_finalize();
>      /* TODO: unref root container, check all devices are ok */
>  
> +    if (exit_script) {
> +        int pid, status;
> +        char *args[5];
> +
> +        /* try to launch network script */
> +        pid = fork();
> +        if (pid < 0) {
> +            error_report("could not launch exit script '%s'", exit_script);
> +            exit(1);
> +        }
> +        if (pid == 0) {
> +            int open_max = sysconf(_SC_OPEN_MAX), i;
> +
> +            for (i = 3; i < open_max; i++) {
> +                close(i);
> +            }
> +            args[0] = (char *)exit_script;
> +            args[1] = g_strdup_printf("%d", shutdown_reason);
> +            args[2] = g_strdup_printf("%d", shutdown_was_reset);
> +            args[3] = qemu_get_vm_name();
> +            args[4] = NULL;
> +            execv(exit_script, args);
> +            _exit(1);
> +        } else {
> +            while (waitpid(pid, &status, 0) != pid) {
> +                /* loop */
> +            }
> +
> +            if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) {
> +                error_report("exit script '%s' failed with status %d",
> +                             exit_script, status);
> +                exit(1);
> +            }
> +        }
> +    }

Our main() function is already huge, so please put this into a separate
function instead (or even better, refactor the launch_script function as
you've mentioned it in the cover letter).

 Thanks,
  Thomas

Re: [Qemu-devel] [PATCH 0/1] add exit-script option to qemu

[Philippe Mathieu-Daudé]

Hi Dominik,

On 03/10/2018 11:13, Dominik Csapak wrote:
> this patch aims to execute a script when qemu exits
> so that one can do cleanups when using --daemonize without
> having to use the qmp monitor
> 
> for now i have mostly copied the script execution code from
> the launch_script function of net/tap.c as i am not sure
> if it would make sense to refactor that and if it does, where
> to put such a function

I'd rather refactor a 'more of 10 common LOC function'.

Maybe qemu_launch_script() in "qemu/osdep.h"?

Phil.

Re: [Qemu-devel] [PATCH 0/1] add exit-script option to qemu

[Daniel P. Berrangé]

On Wed, Oct 03, 2018 at 11:13:43AM +0200, Dominik Csapak wrote:
> this patch aims to execute a script when qemu exits
> so that one can do cleanups when using --daemonize without
> having to use the qmp monitor

IMHO the idea of cleanup scripts run by QEMU itself is flawed.
QEMU will inevitably crash before cleanup scripts can be run,
so whatever mgmt app is using QEMU needs to be able to do
cleanup without QEMU's help.

I think this can be done more reliably with a wrapper script,
that spawns QEMU, waits for it to exit and then calls the
cleanup script. On Linux at least you can use prctl() with
PR_SET_CHILD_SUBREAPER so you can detect exit'ing of QEMU
even after it has daemonized.

Perhaps we could have such a wrapper script put in the
contrib directory

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 0/1] add exit-script option to qemu

[Dominik Csapak]

On 10/4/18 3:51 PM, Daniel P. Berrangé wrote:
> On Wed, Oct 03, 2018 at 11:13:43AM +0200, Dominik Csapak wrote:
>> this patch aims to execute a script when qemu exits
>> so that one can do cleanups when using --daemonize without
>> having to use the qmp monitor
> 
> IMHO the idea of cleanup scripts run by QEMU itself is flawed.
> QEMU will inevitably crash before cleanup scripts can be run,
> so whatever mgmt app is using QEMU needs to be able to do
> cleanup without QEMU's help.
> 
> I think this can be done more reliably with a wrapper script,
> that spawns QEMU, waits for it to exit and then calls the
> cleanup script. On Linux at least you can use prctl() with
> PR_SET_CHILD_SUBREAPER so you can detect exit'ing of QEMU
> even after it has daemonized.
> 
> Perhaps we could have such a wrapper script put in the
> contrib directory
> 
> Regards,
> Daniel
> 
Hi,

for cleaning up after qemu crashes, you are completely right,
(ignoring that the downscript for tap devices also never gets executed
then), but this series has another use.

With it, a user can determine the reason of a graceful shutdown
(e.g., if it was by a signal, qmp or from inside) of qemu,
especially when using -no-reboot without using qmp

and using qmp for that is not very practical for everyone,
or is there another way for that which i am missing?

Regards,
Dominik

Re: [Qemu-devel] [PATCH 0/1] add exit-script option to qemu

[Daniel P. Berrangé]

On Fri, Oct 05, 2018 at 08:56:27AM +0200, Dominik Csapak wrote:
> On 10/4/18 3:51 PM, Daniel P. Berrangé wrote:
> > On Wed, Oct 03, 2018 at 11:13:43AM +0200, Dominik Csapak wrote:
> > > this patch aims to execute a script when qemu exits
> > > so that one can do cleanups when using --daemonize without
> > > having to use the qmp monitor
> > 
> > IMHO the idea of cleanup scripts run by QEMU itself is flawed.
> > QEMU will inevitably crash before cleanup scripts can be run,
> > so whatever mgmt app is using QEMU needs to be able to do
> > cleanup without QEMU's help.
> > 
> > I think this can be done more reliably with a wrapper script,
> > that spawns QEMU, waits for it to exit and then calls the
> > cleanup script. On Linux at least you can use prctl() with
> > PR_SET_CHILD_SUBREAPER so you can detect exit'ing of QEMU
> > even after it has daemonized.
> > 
> > Perhaps we could have such a wrapper script put in the
> > contrib directory
> > 
> > Regards,
> > Daniel
> > 
> Hi,
> 
> for cleaning up after qemu crashes, you are completely right,
> (ignoring that the downscript for tap devices also never gets executed
> then), but this series has another use.
> 
> With it, a user can determine the reason of a graceful shutdown
> (e.g., if it was by a signal, qmp or from inside) of qemu,
> especially when using -no-reboot without using qmp
> 
> and using qmp for that is not very practical for everyone,
> or is there another way for that which i am missing?

Honestly QMP *is* the right answer. We've put alot of effort into QMP
and I don't think it is sensible to start adding new mechanisms to
provide the same information in an adhoc manner.

What makes you think QMP isn't practical to use ?  We have client
impls that talk to QMP in scripts/qmp that are just a few 100 lines
of pretty simple python code.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 0/1] add exit-script option to qemu

[Dominik Csapak]

On 10/5/18 10:38 AM, Daniel P. Berrangé wrote:
> On Fri, Oct 05, 2018 at 08:56:27AM +0200, Dominik Csapak wrote:
>> On 10/4/18 3:51 PM, Daniel P. Berrangé wrote:
>>> On Wed, Oct 03, 2018 at 11:13:43AM +0200, Dominik Csapak wrote:
>>>> this patch aims to execute a script when qemu exits
>>>> so that one can do cleanups when using --daemonize without
>>>> having to use the qmp monitor
>>>
>>> IMHO the idea of cleanup scripts run by QEMU itself is flawed.
>>> QEMU will inevitably crash before cleanup scripts can be run,
>>> so whatever mgmt app is using QEMU needs to be able to do
>>> cleanup without QEMU's help.
>>>
>>> I think this can be done more reliably with a wrapper script,
>>> that spawns QEMU, waits for it to exit and then calls the
>>> cleanup script. On Linux at least you can use prctl() with
>>> PR_SET_CHILD_SUBREAPER so you can detect exit'ing of QEMU
>>> even after it has daemonized.
>>>
>>> Perhaps we could have such a wrapper script put in the
>>> contrib directory
>>>
>>> Regards,
>>> Daniel
>>>
>> Hi,
>>
>> for cleaning up after qemu crashes, you are completely right,
>> (ignoring that the downscript for tap devices also never gets executed
>> then), but this series has another use.
>>
>> With it, a user can determine the reason of a graceful shutdown
>> (e.g., if it was by a signal, qmp or from inside) of qemu,
>> especially when using -no-reboot without using qmp
>>
>> and using qmp for that is not very practical for everyone,
>> or is there another way for that which i am missing?
> 
> Honestly QMP *is* the right answer. We've put alot of effort into QMP
> and I don't think it is sensible to start adding new mechanisms to
> provide the same information in an adhoc manner.
> 
> What makes you think QMP isn't practical to use ?  We have client
> impls that talk to QMP in scripts/qmp that are just a few 100 lines
> of pretty simple python code.
> 

ok, i just found that having to start an extra program waiting for qmp
events might be overkill for some users

nonetheless, i just found out that even with qmp, there is no way
to see if a machine started with '-no-reboot' was trying to reboot
or just shutting down, in both cases i got a SHUTDOWN event
with 'guest: true'

would it make sense to send a patch that introduces a new data field
for the shutdown event that says if it was really a reset?

Re: [Qemu-devel] [PATCH 0/1] add exit-script option to qemu

[Daniel P. Berrangé]

On Fri, Oct 05, 2018 at 02:57:03PM +0200, Dominik Csapak wrote:
> On 10/5/18 10:38 AM, Daniel P. Berrangé wrote:
> > On Fri, Oct 05, 2018 at 08:56:27AM +0200, Dominik Csapak wrote:
> > > On 10/4/18 3:51 PM, Daniel P. Berrangé wrote:
> > > > On Wed, Oct 03, 2018 at 11:13:43AM +0200, Dominik Csapak wrote:
> > > > > this patch aims to execute a script when qemu exits
> > > > > so that one can do cleanups when using --daemonize without
> > > > > having to use the qmp monitor
> > > > 
> > > > IMHO the idea of cleanup scripts run by QEMU itself is flawed.
> > > > QEMU will inevitably crash before cleanup scripts can be run,
> > > > so whatever mgmt app is using QEMU needs to be able to do
> > > > cleanup without QEMU's help.
> > > > 
> > > > I think this can be done more reliably with a wrapper script,
> > > > that spawns QEMU, waits for it to exit and then calls the
> > > > cleanup script. On Linux at least you can use prctl() with
> > > > PR_SET_CHILD_SUBREAPER so you can detect exit'ing of QEMU
> > > > even after it has daemonized.
> > > > 
> > > > Perhaps we could have such a wrapper script put in the
> > > > contrib directory
> > > > 
> > > > Regards,
> > > > Daniel
> > > > 
> > > Hi,
> > > 
> > > for cleaning up after qemu crashes, you are completely right,
> > > (ignoring that the downscript for tap devices also never gets executed
> > > then), but this series has another use.
> > > 
> > > With it, a user can determine the reason of a graceful shutdown
> > > (e.g., if it was by a signal, qmp or from inside) of qemu,
> > > especially when using -no-reboot without using qmp
> > > 
> > > and using qmp for that is not very practical for everyone,
> > > or is there another way for that which i am missing?
> > 
> > Honestly QMP *is* the right answer. We've put alot of effort into QMP
> > and I don't think it is sensible to start adding new mechanisms to
> > provide the same information in an adhoc manner.
> > 
> > What makes you think QMP isn't practical to use ?  We have client
> > impls that talk to QMP in scripts/qmp that are just a few 100 lines
> > of pretty simple python code.
> > 
> 
> ok, i just found that having to start an extra program waiting for qmp
> events might be overkill for some users
> 
> nonetheless, i just found out that even with qmp, there is no way
> to see if a machine started with '-no-reboot' was trying to reboot
> or just shutting down, in both cases i got a SHUTDOWN event
> with 'guest: true'
> 
> would it make sense to send a patch that introduces a new data field
> for the shutdown event that says if it was really a reset?

I had a feeling there was another way to detct it, but I'm not seeing
it in QMP. So yeah, if this can't be determined, then I expect it is
worth extending QMP to report this 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|