[libvirt] [RFC v2] Export KVM Host Power Management capabilities

[Eric Blake]

On 08/05/2011 05:54 AM, Srivatsa S. Bhat wrote:
> This patch exports KVM Host Power Management capabilities as XML so that
> higher-level systems management software can make use of these features
> available in the host.
>
> The script "pm-is-supported" (from pm-utils package) is run to discover if
> Suspend-to-RAM (S3) or Suspend-to-Disk (S4) is supported by the host.
> If either of them are supported, then a new tag "<power_management>" is
> introduced in the XML under the<host>  tag.
>
>      </cpu>
>      <power_management>          <<<=== New host power management features
>        <S3/>
>        <S4/>
>      </power_management>

Nice.

>
> However in case the host does not support any power management feature,
> then the XML will not contain the<power_management>  tag.

Wouldn't it be better to include <power_management/> (ie. an empty tag) 
to explicitly document that power management capabilities were 
successfully probed but none found, and leave the case of omitted 
<power_management> to imply that no probe was done in the first place 
(either because libvirt predates this xml addition, or because 
pm-is-supported is not found)?


>   src/conf/capabilities.c      |   34 +++++++++++++++++++++++++++
>   src/conf/capabilities.h      |    7 ++++++
>   src/libvirt_private.syms     |    2 ++
>   src/qemu/qemu_capabilities.c |   10 ++++++++
>   src/util/util.c              |   52 ++++++++++++++++++++++++++++++++++++++++++
>   src/util/util.h              |    7 ++++++
>   6 files changed, 112 insertions(+), 0 deletions(-)

Incomplete - this also needs to touch docs/schemas/capability.rng to 
validate the new XML in the rng grammar, as well as 
docs/formatcaps.html.in to at least demonstrate the new XML in the 
red-shaded portion of the example (actually, that page really needs some 
TLC, because it is lacking lots of details about the available XML, but 
that's an independent project).

>
> +/**
> + * virCapabilitiesAddHostPowerManagement:
> + * @caps: capabilities to extend
> + * @name: name of power management feature
> + *
> + * Registers a new host power management feature, eg: 'S3' or 'S4'
> + */
> +int
> +virCapabilitiesAddHostPowerManagement(virCapsPtr caps,
> +                                      const char *name)
> +{
> +    if(VIR_RESIZE_N(caps->host.powerMgmt, caps->host.npowerMgmt_max,
> +                   caps->host.npowerMgmt, 1)<  0)
> +        return -1;
> +
> +    if((caps->host.powerMgmt[caps->host.npowerMgmt] = strdup(name)) == NULL)
> +        return -1;

This returns -1 without calling virReportOOMError(),...

> @@ -686,6 +711,15 @@ virCapabilitiesFormatXML(virCapsPtr caps)
>
>       virBufferAddLit(&xml, "</cpu>\n");
>
> +    if(caps->host.npowerMgmt) {
> +        virBufferAddLit(&xml, "<power_management>\n");
> +        for (i = 0; i<  caps->host.npowerMgmt ; i++) {
> +            virBufferAsprintf(&xml, "<%s/>\n",
> +                              caps->host.powerMgmt[i]);
> +        }
> +        virBufferAddLit(&xml, "</power_management>\n");
> +    }

See my above thoughts - this should support an explicit 
<power_management/> when we were able to successfully probe but found no 
capabilities; which means an extra bool in the _virCapsHost struct.

>       }
>
> +    /* Add the power management features of the host */
> +
> +    /* Check for Suspend-to-RAM support (S3) */
> +    if(virCheckPMCapability(HOST_PM_S3) == 0)
> +        virCapabilitiesAddHostPowerManagement(caps, "S3");

...yet here, you aren't checking the return value for failure.  That's a 
silent bug on OOM, which is not appropriate.  One of the two places 
needs to call virReportOOMError(), and the caller must not discard failure.

> +
> +    /* Check for Suspend-to-Disk support (S4) */
> +    if(virCheckPMCapability(HOST_PM_S4) == 0)
> +        virCapabilitiesAddHostPowerManagement(caps, "S4");

You are manually converting between #define HOST_PM_S* and strings; it 
seems like it would be better to introduce an enum type and use the 
VIR_ENUM magic to make the translation automated, as well as more 
extensible in the future.  And if you do that, then _virCapsHost can 
track an array of enum values instead of an array of strings, for a more 
compact internal representation.

> +/**
> + * Check the Power Management Capabilities of the host system.
> + * The script 'pm-is-supported' (from the pm-utils package) is run
> + * to find out if the capability is supported by the host.
> + *
> + * @capability: capability to check for
> + * HOST_PM_S3: Check for Suspend-to-RAM support
> + * HOST_PM_S4: Check for Suspend-to-Disk support
> + *
> + * Returns 0 if supported, -1 if not supported.

I think this should be:

0 if query successful but unsupported, 1 if supported, and -1 if error 
(such as pm-is-supported not installed).  The error can safely be 
ignored if the caller doesn't care, but having the tri-state return 
value will make it possible to later add any code that explicitly cares.

> + */
> +int
> +virCheckPMCapability(int capability)
> +{
> +
> +    char *path = NULL;
> +    int status = -1, ret = -1;
> +    virCommandPtr cmd;
> +
> +    if((path = virFindFileInPath("pm-is-supported")) == NULL)
> +        return -1;

Should we update the libvirt.spec file to guarantee this dependency on 
Fedora installations?

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org