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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org