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

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

On Fri, May 02, 2014 at 08:01:00AM -0600, Jim Fehlig wrote:
> Daniel P. Berrange wrote:
> > On Thu, May 01, 2014 at 04:14:39PM -0600, Jim Fehlig wrote:
> >   
> >> Add support for VIR_DOMAIN_REBOOT_PARAVIRT and
> >> VIR_DOMAIN_REBOOT_ACPI_POWER_BTN flags in
> >> libxlDomainReboot().
> >>
> >> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> >> ---
> >>  src/libxl/libxl_driver.c | 30 ++++++++++++++++++++++++++----
> >>  1 file changed, 26 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> >> index 28e8512..6c63251 100644
> >> --- a/src/libxl/libxl_driver.c
> >> +++ b/src/libxl/libxl_driver.c
> >> @@ -938,7 +938,11 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
> >>      int ret = -1;
> >>      libxlDomainObjPrivatePtr priv;
> >>  
> >> -    virCheckFlags(0, -1);
> >> +    virCheckFlags(VIR_DOMAIN_REBOOT_ACPI_POWER_BTN |
> >> +                  VIR_DOMAIN_REBOOT_PARAVIRT, -1);
> >> +    if (flags == 0)
> >> +        flags = VIR_DOMAIN_REBOOT_PARAVIRT |
> >> +            VIR_DOMAIN_REBOOT_ACPI_POWER_BTN;
> >>  
> >>      if (!(vm = libxlDomObjFromDomain(dom)))
> >>          goto cleanup;
> >> @@ -953,13 +957,31 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
> >>      }
> >>  
> >>      priv = vm->privateData;
> >> -    if (libxl_domain_reboot(priv->ctx, vm->def->id) != 0) {
> >> +    if (flags & VIR_DOMAIN_REBOOT_PARAVIRT) {
> >> +        ret = libxl_domain_reboot(priv->ctx, vm->def->id);
> >> +        if (ret == 0)
> >> +            goto cleanup;
> >> +
> >> +        if (ret != ERROR_NOPARAVIRT) {
> >> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> >> +                           _("Failed to reboot domain '%d' with libxenlight"),
> >> +                           vm->def->id);
> >> +            ret = -1;
> >> +            goto cleanup;
> >> +        }
> >> +    }
> >> +
> >> +    if (flags & VIR_DOMAIN_REBOOT_ACPI_POWER_BTN) {
> >> +        ret = libxl_send_trigger(priv->ctx, vm->def->id,
> >> +                                 LIBXL_TRIGGER_RESET, 0);
> >>     
> >
> > What does this trigger in ACPI ? IIUC, it'll do a hard reset of the
> > board,
> 
> Yes, I think you are right.  Similar to pushing the reset button.
> 
> >  which is not the same as a controlled reboot which this API
> > wants.  There isn't any ACPI button that I know of that guests will
> > interpret todo a controlled reboot, so in the QEMU driver we actually
> > just send a normal ACPI shutdown event. We have QEMU configured with
> > the '-no-shutdown' flag so when it finishes doing an controlled APCI
> > shutdown, we can then reset the board and start the CPUs again, which
> > gives the illusion of a controlled reboot.
> >
> > Given that Xen has a decent paravirt reboot facility I'd probably
> > just not bother with trying to fake the controlled reboot via ACPI.
> >   
> 
> Ok, that sounds reasonable to me.  I'll drop this patch when pushing the
> others, post 1.2.4.  Should 1/3 retain the VIR_DOMAIN_REBOOT_PARAVIRT
> addition tovirDomainRebootFlagValues?

I don't think you need to drop the patch/code. It is still useful, IMHO,
to have the explicit flag for VIR_DOMAIN_REBOOT_PARAVIRT. I'd just
suggest you remove the block of code for VIR_DOMAIN_REBOOT_ACPI_POWER_BTN
impl in the reboot method.


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 :|