11:18 a.m.
On 08/08/2011 09:18 AM, Srivatsa S. Bhat wrote:
Open issues:
-----------
1. Design new APIs in libvirt to actually exploit the host power management
features instead of relying on external programs. This was discussed in
[4].
As pointed out in [4], until we have additional use for this feature,
then this feature in isolation is unlikely to be pushed. That is not
meant to discourage you from developing further patches, rather it is
just stating that this patch will probably be deferred until it can be
pushed as part of a larger series.
2. Decide on whether to include "pm-utils" package in the
libvirt.spec
file considering the fact that the package name (pm-utils) may differ
from one Linux distribution to another.
No decision to make - libvirt.spec is solely for Fedora, where the
package is always named pm-utils. Other distros use other packaging
files, and can adjust accordingly, it's just that none of those other
packaging files have been contributed for inclusion in upstream libvirt.git.
+ <define name='power_management'>
+ <choice>
No need for a <choice> here.
+ <element name='power_management'>
+ <optional>
+ <element name='S3'>
+ <empty/>
+ </element>
+ </optional>
+ <optional>
+ <element name='S4'>
+ <empty/>
+ </element>
+ </optional>
+ </element>
The two <optional> blocks are sufficient for allowing
<power_management/> as an empty element, without any further rng grammar.
+/**
+ * virCapabilitiesAddHostPowerManagement:
+ * @caps: capabilities to extend
+ * @feature: the power management feature to be added
+ *
+ * Registers a new host power management feature, eg: 'S3' or 'S4'
+ */
+int
+virCapabilitiesAddHostPowerManagement(virCapsPtr caps,
+ int feature)
+{
+ if(VIR_RESIZE_N(caps->host.powerMgmt, caps->host.npowerMgmt_max,
+ caps->host.npowerMgmt, 1)< 0) {
+ virReportOOMError();
+ return -1;
+ }
+
+ caps->host.powerMgmt[caps->host.npowerMgmt] = feature;
+ caps->host.npowerMgmt++;
After thinking about this more, I'm wondering if it would be smarter to
represent power management as a bitmask:
caps->hst.powerMgmt |= 1U << feature;
at which point, you still need isPMQuerySuccess (but see below on
naming), but you don't need npowerMgmt. Instead, output would be:
@@ -686,6 +717,25 @@ virCapabilitiesFormatXML(virCapsPtr caps)
virBufferAddLit(&xml, "</cpu>\n");
+ if(caps->host.isPMQuerySuccess) {
if (caps->host.powerMgmt) {
unsigned int pm = caps->host.powerMgmt;
virBufferAddLit(&xml, "<power_management>\n");
while (pm) {
int bit = ffs(pm) - 1;
virBufferAsprintf(&xml, "<%s/>\n",
virHostPMCapabilityTypeToString(bit);
pm &= ~bit;
}
virBufferAddLit(&xml, "</power_management>\n");
} else {
virBufferAddLit(&xml, "<power_management/>\n");
}
+++ b/src/conf/capabilities.h
@@ -105,6 +105,10 @@ struct _virCapsHost {
size_t nfeatures;
size_t nfeatures_max;
char **features;
+ bool isPMQuerySuccess;
This isn't typical naming. For consistency, I'm thinking:
bool powerMgmt_valid;
defaulting to false (powerMgmt is irrelavant), but set to true when
powerMgmt contains valid data (where 0 can include valid data).
+ size_t npowerMgmt;
+ size_t npowerMgmt_max;
+ int *powerMgmt; /* enum virHostPMCapability */
See above about just using 'unsigned int powerMgmt' as a bit-mask of
valid capabilities, rather than a dynamically allocated array of ints.
@@ -824,6 +825,32 @@ virCapsPtr qemuCapsInit(virCapsPtr old_caps)
old_caps->host.cpu = NULL;
}
+ /* Add the power management features of the host */
+
+ /* Check for Suspend-to-RAM support (S3) */
+ status = virCheckPMCapability(VIR_HOST_PM_S3);
+ if(status< 0) {
+ caps->host.isPMQuerySuccess = false;
+ VIR_WARN("Failed to get host power management features");
+ } else {
+ /* The PM Query succeeded */
+ caps->host.isPMQuerySuccess = true;
+ if(status == 1) /* S3 is supported */
+ virCapabilitiesAddHostPowerManagement(caps, VIR_HOST_PM_S3);
+ }
+
+ /* Check for Suspend-to-Disk support (S4) */
+ status = virCheckPMCapability(VIR_HOST_PM_S4);
+ if(status< 0) {
+ caps->host.isPMQuerySuccess = false;
+ VIR_WARN("Failed to get host power management features");
+ } else {
+ /* The PM Query succeeded */
+ caps->host.isPMQuerySuccess = true;
+ if(status == 1) /* S4 is supported */
+ virCapabilitiesAddHostPowerManagement(caps, VIR_HOST_PM_S4);
+ }
You don't want to set caps->host.isPMQuerySuccess (by whatever name) to
true unless both commands succeeded. If the first command succeeds but
the second fails, then you are better off treating the entire operation
as failed.
Hmm, maybe virCheckPMCapability should take no arguments, and return the
bitmask directly, rather than making qemuCapsInit have to compute the
right bitmask.
}
+
+/**
+ * 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
+ * VIR_HOST_PM_S3: Check for Suspend-to-RAM support
+ * VIR_HOST_PM_S4: Check for Suspend-to-Disk support
+ *
+ * Return values:
+ * 1 if the capability is supported.
+ * 0 if the query was successful but the capability is
+ * not supported by the host.
+ * -1 on error like 'pm-is-supported' is not found.
+ */
+int
+virCheckPMCapability(int capability)
+{
As I mentioned above, it would be nicer to have this return the bitmask
up front, rather than to make each caller have to repeatedly call this
function for as many capabilities as are currently defined. -1 for
unknown, 0 for success but no support, or positive for a bitmask of
1<<bit for each supported bit.
+++ b/src/util/virterror.c
@@ -148,6 +148,9 @@ static const char *virErrorDomainName(virErrorDomain domain) {
case VIR_FROM_CPU:
dom = "CPU ";
break;
+ case VIR_FROM_CAPABILITIES:
+ dom = "Capabilities ";
+ break;
case VIR_FROM_NWFILTER:
dom = "Network Filter ";
break;
Better to make these case statements line up with declaration order
(VIR_FROM_CAPABILITIES is new, so it should be at the end of the list
just before default:).
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org