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

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?