[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/4] Add new virDomainShutdownFlags API



On 01/17/2012 04:44 AM, Michal Privoznik wrote:
> Add a new API virDomainShutdownFlags and define:
> 
>     VIR_DOMAIN_SHUTDOWN_DEFAULT        = 0,
>     VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN = (1 << 0),
>     VIR_DOMAIN_SHUTDOWN_GUEST_AGENT    = (1 << 1),
> 
> Also define some flags for the reboot API
> 
>     VIR_DOMAIN_REBOOT_DEFAULT        = 0,
>     VIR_DOMAIN_REBOOT_ACPI_POWER_BTN = (1 << 0),
>     VIR_DOMAIN_REBOOT_GUEST_AGENT    = (1 << 1),
> 
> Although these two APIs currently have the same flags, using
> separate enums allows them to expand separately in the future.

Fair enough.

> 
> Add stub impls of the new API for all existing drivers
> ---

I might have split this into several patches, but I think doing it in
one go is okay since the stubs are trivial.

>  include/libvirt/libvirt.h.in |   15 +++++++++
>  src/driver.h                 |    5 +++
>  src/libvirt.c                |   65 +++++++++++++++++++++++++++++++++++++++++-
>  src/libvirt_public.syms      |    4 ++

That is, I might have done part 1 (the public API),

>  src/remote/remote_driver.c   |    1 +
>  src/remote/remote_protocol.x |    8 ++++-
>  src/remote_protocol-structs  |    5 +++

part 2 (the RPC),

>  src/esx/esx_driver.c         |   11 ++++++-
>  src/libxl/libxl_driver.c     |   12 +++++++-
>  src/openvz/openvz_driver.c   |    1 +
>  src/test/test_driver.c       |   11 ++++++-
>  src/uml/uml_driver.c         |   11 ++++++-
>  src/vbox/vbox_tmpl.c         |   11 ++++++-
>  src/vmware/vmware_driver.c   |    1 +
>  src/xen/xen_driver.c         |   14 ++++++++-
>  src/xenapi/xenapi_driver.c   |   12 +++++++-

and part 3 (the stubs).

> +++ b/include/libvirt/libvirt.h.in
> @@ -1148,7 +1148,22 @@ virDomainPtr            virDomainLookupByUUID   (virConnectPtr conn,
>  virDomainPtr            virDomainLookupByUUIDString     (virConnectPtr conn,
>                                                          const char *uuid);
>  
> +typedef enum {
> +    VIR_DOMAIN_SHUTDOWN_DEFAULT        = 0,
> +    VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN = (1 << 0),
> +    VIR_DOMAIN_SHUTDOWN_GUEST_AGENT    = (1 << 1),
> +} virDomainShutdownFlagValues;

No documentation comments?  Even a /** doc */ prior to the typedef that
mentions virDomainShutdownFlagValues might be helpful to the doc
generation process.

> +++ b/src/libvirt.c
> @@ -3106,14 +3106,77 @@ error:
>  }
>  
>  /**
> + * virDomainShutdownFlags:
> + * @domain: a domain object
> + * @flags: bitwise-OR of virDomainShutdownFlagValues
> + *
> + * Shutdown a domain, the domain object is still usable thereafter but
> + * the domain OS is being stopped. Note that the guest OS may ignore the
> + * request.  For guests that react to a shutdown request, the differences
> + * from virDomainDestroy() are that the guest's disk storage will be in a
> + * stable state rather than having the (virtual) power cord pulled, and
> + * this command returns as soon as the shutdown request is issued rather
> + * than blocking until the guest is no longer running.
> + *
> + * If the domain is transient and has any snapshot metadata (see
> + * virDomainSnapshotNum()), then that metadata will automatically
> + * be deleted when the domain quits.
> + *
> + * If @flags is set to zero, then the hypervisor will chose the
> + * method of shutdown it considers best. To have greater control
> + * pass one of the virDomainShutdownFlagValues.

Maybe mention the existing flag names:

Use VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN to trigger a shutdown through an
ACPI interrupt, and VIR_DOMAIN_SHUTDOWN_GUEST_AGENT to trigger a
shutdown through a guest agent call (this requires that the domain have
a <channel> device appropriately wired to a guest agent).

Is it an error if multiple flags are requested at once?

> +/**
>   * virDomainReboot:
>   * @domain: a domain object
> - * @flags: extra flags; not used yet, so callers should always pass 0
> + * @flags: bitwise-OR of virDomainRebootFlagValues
>   *
>   * Reboot a domain, the domain object is still usable there after but
>   * the domain OS is being stopped for a restart.
>   * Note that the guest OS may ignore the request.
>   *
> + * If @flags is set to zero, then the hypervisor will chose the
> + * method of shutdown it considers best. To have greater control
> + * pass one of the virDomainRebootFlagValues.

Again, mention the possible flag values, and that an agent request
requires additional support from the XML.

> +++ b/src/openvz/openvz_driver.c
> @@ -1693,6 +1693,7 @@ static virDriver openvzDriver = {
>      .domainSuspend = openvzDomainSuspend, /* 0.8.3 */
>      .domainResume = openvzDomainResume, /* 0.8.3 */
>      .domainShutdown = openvzDomainShutdown, /* 0.3.1 */
> +    .domainShutdownFlags = openvzDomainShutdownFlags, /* 0.9.10 */

Hmm.  Are we setting ourselves up for issues down the road by sharing
shutdown and destroy implementations for openvz?  But it's a
pre-existing problem, not made worse by the patch, and okay as long as
we reject all flag values.

> +++ b/src/vmware/vmware_driver.c
> @@ -980,6 +980,7 @@ static virDriver vmwareDriver = {
>      .domainSuspend = vmwareDomainSuspend, /* 0.8.7 */
>      .domainResume = vmwareDomainResume, /* 0.8.7 */
>      .domainShutdown = vmwareDomainShutdown, /* 0.8.7 */
> +    .domainShutdownFlags = vmwareDomainShutdownFlags, /* 0.9.10 */
>      .domainReboot = vmwareDomainReboot, /* 0.8.7 */
>      .domainDestroy = vmwareDomainShutdown, /* 0.8.7 */
>      .domainDestroyFlags = vmwareDomainShutdownFlags, /* 0.9.4 */

Same potential issue as openvz.  Oh well, not worth worrying about today.

ACK with documentation findings addressed.

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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]