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@xxxxxxxxxxxxxxxxxx>
Linux Technology Center,
IBM India Systems and Technology Lab
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list