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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list