Re: [libvirt] [PATCH V2 2/3] libxl: support PARAVIRT and ACPI shutdown flags

["Daniel P. Berrange" <berrange@redhat.com>]

On Thu, May 01, 2014 at 04:14:38PM -0600, Jim Fehlig wrote:
> Add support for VIR_DOMAIN_SHUTDOWN_PARAVIRT and
> VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN flags in
> libxlDomainShutdownFlags().
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
>  src/libxl/libxl_driver.c | 34 ++++++++++++++++++++++++++--------
>  1 file changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index dcc3ac2..28e8512 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -873,7 +873,11 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
>      int ret = -1;
>      libxlDomainObjPrivatePtr priv;
>  
> -    virCheckFlags(0, -1);
> +    virCheckFlags(VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN |
> +                  VIR_DOMAIN_SHUTDOWN_PARAVIRT, -1);
> +    if (flags == 0)
> +        flags = VIR_DOMAIN_SHUTDOWN_PARAVIRT |
> +            VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN;
>  
>      if (!(vm = libxlDomObjFromDomain(dom)))
>          goto cleanup;
> @@ -888,18 +892,32 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
>      }
>  
>      priv = vm->privateData;
> -    if (libxl_domain_shutdown(priv->ctx, vm->def->id) != 0) {
> +    if (flags & VIR_DOMAIN_SHUTDOWN_PARAVIRT) {
> +        ret = libxl_domain_shutdown(priv->ctx, vm->def->id);
> +        if (ret == 0)
> +            goto cleanup;
> +
> +        if (ret != ERROR_NOPARAVIRT) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Failed to shutdown domain '%d' with libxenlight"),
> +                           vm->def->id);
> +            ret = -1;
> +            goto cleanup;
> +        }

I think you need 'ret = -1' here too, otherwise if domain shutdown
fails, but flags does not request ACPI, then we can leak
ret == ERROR_NOPARAVIRT back to the public API.

> +    }
> +
> +    if (flags & VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN) {
> +        ret = libxl_send_trigger(priv->ctx, vm->def->id,
> +                                 LIBXL_TRIGGER_POWER, 0);
> +        if (ret == 0)
> +            goto cleanup;
> +
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Failed to shutdown domain '%d' with libxenlight"),
>                         vm->def->id);
> -        goto cleanup;
> +        ret = -1;
>      }
>  
> -    /* vm is marked shutoff (or removed from domains list if not persistent)
> -     * in shutdown event handler.
> -     */
> -    ret = 0;
> -
>   cleanup:
>      if (vm)
>          virObjectUnlock(vm);

ACK to the patch with that fix added  (post 1.2.4 release of course)


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|