[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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?

--
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]