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

[Srivatsa S. Bhat]

On 08/05/2011 09:21 PM, Eric Blake wrote:
> 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?
> 

Hi,

Thank you Eric for your kind review. I'll post the next iteration of the
patch by incorporating most of the changes you have suggested.

-- 
Regards,
Srivatsa S. Bhat  <srivatsa.bhat at linux.vnet.ibm.com>
Linux Technology Center,
IBM India Systems and Technology Lab