For abnormal shutoff reasons, we can start guest again with qemu hooks.

Signed-off-by: Sun Feng <loyou85@gmail.com>
---
 docs/hooks.rst          | 3 ++-
 src/qemu/qemu_process.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/docs/hooks.rst b/docs/hooks.rst
index 1dbc492bd4..45856e4ca4 100644
--- a/docs/hooks.rst
+++ b/docs/hooks.rst
@@ -204,7 +204,8 @@ operation. There is no specific operation to indicate a "restart" is occurring.
 
 -  When a QEMU guest is stopped, the qemu hook script is called in two
    locations, to match the startup. First, :since:`since 0.8.0`, the hook is
-   called before libvirt restores any labels:
+   called before libvirt restores any labels, :since:`since 9.10.0`, shutoff
+   reason is delivered with **extra argument**:
 
    ::
 
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e4bcb628cf..c42f5c9139 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8564,7 +8564,8 @@ void qemuProcessStop(virQEMUDriver *driver,
         /* we can't stop the operation even if the script raised an error */
         ignore_value(virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name,
                                  VIR_HOOK_QEMU_OP_STOPPED, VIR_HOOK_SUBOP_END,
-                                 NULL, xml, NULL));
+                                 virDomainShutoffReasonTypeToString(reason),
+                                 xml, NULL));
     }
 
     /* Reset Security Labels unless caller don't want us to */
-- 
2.34.1
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Wed, Apr 17, 2024 at 13:58:06 +0800, Sun Feng wrote:
> For abnormal shutoff reasons, we can start guest again with qemu hooks.

Beware that:

  A hook script must not call back into libvirt, as the libvirt daemon is already waiting for the script to exit.

  A deadlock is likely to occur.

https://libvirt.org/hooks.html#calling-libvirt-functions-from-within-a-hook-script

Preferrably summarize what the patch is doing or suggest something that
is not strongly discouraged by the docs.

> Signed-off-by: Sun Feng <loyou85@gmail.com>
> ---
>  docs/hooks.rst          | 3 ++-
>  src/qemu/qemu_process.c | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/hooks.rst b/docs/hooks.rst
> index 1dbc492bd4..45856e4ca4 100644
> --- a/docs/hooks.rst
> +++ b/docs/hooks.rst
> @@ -204,7 +204,8 @@ operation. There is no specific operation to indicate a "restart" is occurring.
>  
>  -  When a QEMU guest is stopped, the qemu hook script is called in two
>     locations, to match the startup. First, :since:`since 0.8.0`, the hook is
> -   called before libvirt restores any labels:
> +   called before libvirt restores any labels, :since:`since 9.10.0`, shutoff
> +   reason is delivered with **extra argument**:

Next release is going to be 10.3.0

>  
>     ::
>  
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index e4bcb628cf..c42f5c9139 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8564,7 +8564,8 @@ void qemuProcessStop(virQEMUDriver *driver,
>          /* we can't stop the operation even if the script raised an error */
>          ignore_value(virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name,
>                                   VIR_HOOK_QEMU_OP_STOPPED, VIR_HOOK_SUBOP_END,
> -                                 NULL, xml, NULL));
> +                                 virDomainShutoffReasonTypeToString(reason),
> +                                 xml, NULL));

To be honest I'm not exactly sold that this is needed as the reason for
this hook's existence is to simply clear up any resources that would be
set up by the start hook, thus it's usually irrelevant why that
happened.

In your example you need to setup the hook so that it spawns off a
separate process and terminates thus you can also query the shutdown
reason from libvirt in that call too.

Said that I'm not strongly against it, I'm just pointing out that I
don't really see a good reason based on the fact that the hooks must not
call back into libvirt.

I'll let others chime in with their opinion.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Wed, Apr 17, 2024 at 10:08:24AM +0200, Peter Krempa wrote:
> On Wed, Apr 17, 2024 at 13:58:06 +0800, Sun Feng wrote:
> > For abnormal shutoff reasons, we can start guest again with qemu hooks.
> 
> Beware that:
> 
>   A hook script must not call back into libvirt, as the libvirt daemon is already waiting for the script to exit.
> 
>   A deadlock is likely to occur.
> 
> https://libvirt.org/hooks.html#calling-libvirt-functions-from-within-a-hook-script
> 
> Preferrably summarize what the patch is doing or suggest something that
> is not strongly discouraged by the docs.
> 
> > Signed-off-by: Sun Feng <loyou85@gmail.com>
> > ---
> >  docs/hooks.rst          | 3 ++-
> >  src/qemu/qemu_process.c | 3 ++-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/docs/hooks.rst b/docs/hooks.rst
> > index 1dbc492bd4..45856e4ca4 100644
> > --- a/docs/hooks.rst
> > +++ b/docs/hooks.rst
> > @@ -204,7 +204,8 @@ operation. There is no specific operation to indicate a "restart" is occurring.
> >  
> >  -  When a QEMU guest is stopped, the qemu hook script is called in two
> >     locations, to match the startup. First, :since:`since 0.8.0`, the hook is
> > -   called before libvirt restores any labels:
> > +   called before libvirt restores any labels, :since:`since 9.10.0`, shutoff
> > +   reason is delivered with **extra argument**:
> 
> Next release is going to be 10.3.0
> 
> >  
> >     ::
> >  
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index e4bcb628cf..c42f5c9139 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -8564,7 +8564,8 @@ void qemuProcessStop(virQEMUDriver *driver,
> >          /* we can't stop the operation even if the script raised an error */
> >          ignore_value(virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name,
> >                                   VIR_HOOK_QEMU_OP_STOPPED, VIR_HOOK_SUBOP_END,
> > -                                 NULL, xml, NULL));
> > +                                 virDomainShutoffReasonTypeToString(reason),
> > +                                 xml, NULL));
> 
> To be honest I'm not exactly sold that this is needed as the reason for
> this hook's existence is to simply clear up any resources that would be
> set up by the start hook, thus it's usually irrelevant why that
> happened.

The thing that would convince me is that in outgoing migration there
are some resources you might wish to skip cleanup of if they need
to be preserved in some manner for the dst host. IIUC we don't
currently have a way to indicate that the "stopped" hook is in
response to an outgoing migration, and the "reason" would provide
this.

With that justification though, I would now query the asymmetry of
only providing 'reason' to the 'stopped' hook. It should be provided
to *all* the hooks - prepare, start, started, stop, stopped, release.

With 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 :|
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org