[libvirt] [PATCH 00/14] Cleanup node suspend code
This patch series does a large number of pretty small / trivial cleanups of the recently committed node suspend code. There are a few important changes though - Patch 1 changes public API enum values - Patch 3 changes the capabilities XML schema - Patch 11 ensures we don't probe suspend info unles we need it - Patch 14 fixes a bug Obviously due to 1 & 3 we need to do this before the next release
From: "Daniel P. Berrange" <berrange@redhat.com> The VIR_NODE_SUSPEND_TARGET constants are not flags, so they should just be assigned straightforward incrementing values. * include/libvirt/libvirt.h.in: Change VIR_NODE_SUSPEND_TARGET values --- include/libvirt/libvirt.h.in | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index f8ca5cf..f32b197 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -257,9 +257,12 @@ typedef enum { * transitioned to. */ typedef enum { - VIR_NODE_SUSPEND_TARGET_MEM = (1 << 0), - VIR_NODE_SUSPEND_TARGET_DISK = (1 << 1), - VIR_NODE_SUSPEND_TARGET_HYBRID = (1 << 2), + VIR_NODE_SUSPEND_TARGET_MEM = 0, + VIR_NODE_SUSPEND_TARGET_DISK = 1, + VIR_NODE_SUSPEND_TARGET_HYBRID = 2, + + /* This constant is subject to change */ + VIR_NODE_SUSPEND_TARGET_LAST, } virNodeSuspendTarget; /** -- 1.7.6.4
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The VIR_NODE_SUSPEND_TARGET constants are not flags, so they should just be assigned straightforward incrementing values.
* include/libvirt/libvirt.h.in: Change VIR_NODE_SUSPEND_TARGET values --- include/libvirt/libvirt.h.in | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index f8ca5cf..f32b197 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -257,9 +257,12 @@ typedef enum { * transitioned to. */ typedef enum { - VIR_NODE_SUSPEND_TARGET_MEM = (1 << 0), - VIR_NODE_SUSPEND_TARGET_DISK = (1 << 1), - VIR_NODE_SUSPEND_TARGET_HYBRID = (1 << 2), + VIR_NODE_SUSPEND_TARGET_MEM = 0, + VIR_NODE_SUSPEND_TARGET_DISK = 1, + VIR_NODE_SUSPEND_TARGET_HYBRID = 2, + + /* This constant is subject to change */ + VIR_NODE_SUSPEND_TARGET_LAST,
ACK. And agree that it is an API-changer, so it must go in now before we release and solidify the API. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> The internal virHostPMCapability enum just duplicates the public virNodeSuspendTarget enum, but with different names. * src/util/util.c: Use VIR_NODE_SUSPEND_TARGET constants * src/util/util.h: Remove virHostPMCapability enum * src/conf/capabilities.c: Use VIR_NODE_SUSPEND_TARGET_LAST --- src/conf/capabilities.c | 2 +- src/util/util.c | 18 +++++++++--------- src/util/util.h | 8 -------- 3 files changed, 10 insertions(+), 18 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 70f9ab0..a2ca46b 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -36,7 +36,7 @@ #define VIR_FROM_THIS VIR_FROM_CAPABILITIES -VIR_ENUM_IMPL(virHostPMCapability, VIR_HOST_PM_LAST, +VIR_ENUM_IMPL(virHostPMCapability, VIR_NODE_SUSPEND_TARGET_LAST, "S3", "S4", "Hybrid-Suspend") /** diff --git a/src/util/util.c b/src/util/util.c index 34541ec..5bd8c91 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2628,9 +2628,9 @@ virTypedParameterArrayClear(virTypedParameterPtr params, int nparams) * the query * @feature: The power management feature to check whether it is supported * by the host. Values could be: - * VIR_HOST_PM_S3 for Suspend-to-RAM - * VIR_HOST_PM_S4 for Suspend-to-Disk - * VIR_HOST_PM_HYBRID_SUSPEND for Hybrid-Suspend + * VIR_NODE_SUSPEND_TARGET_MEM + * VIR_NODE_SUSPEND_TARGET_DISK + * VIR_NODE_SUSPEND_TARGET_HYBRID * * Run the script 'pm-is-supported' (from the pm-utils package) * to find out if @feature is supported by the host. @@ -2645,13 +2645,13 @@ virDiscoverHostPMFeature(unsigned int *bitmask, unsigned int feature) int ret = -1; switch (feature) { - case VIR_HOST_PM_S3: + case VIR_NODE_SUSPEND_TARGET_MEM: cmd = virCommandNewArgList("pm-is-supported", "--suspend", NULL); break; - case VIR_HOST_PM_S4: + case VIR_NODE_SUSPEND_TARGET_DISK: cmd = virCommandNewArgList("pm-is-supported", "--hibernate", NULL); break; - case VIR_HOST_PM_HYBRID_SUSPEND: + case VIR_NODE_SUSPEND_TARGET_HYBRID: cmd = virCommandNewArgList("pm-is-supported", "--suspend-hybrid", NULL); break; default: @@ -2695,17 +2695,17 @@ virGetPMCapabilities(unsigned int *bitmask) *bitmask = 0; /* Check support for Suspend-to-RAM (S3) */ - ret = virDiscoverHostPMFeature(bitmask, VIR_HOST_PM_S3); + ret = virDiscoverHostPMFeature(bitmask, VIR_NODE_SUSPEND_TARGET_MEM); if (ret < 0) return -1; /* Check support for Suspend-to-Disk (S4) */ - ret = virDiscoverHostPMFeature(bitmask, VIR_HOST_PM_S4); + ret = virDiscoverHostPMFeature(bitmask, VIR_NODE_SUSPEND_TARGET_DISK); if (ret < 0) return -1; /* Check support for Hybrid-Suspend */ - ret = virDiscoverHostPMFeature(bitmask, VIR_HOST_PM_HYBRID_SUSPEND); + ret = virDiscoverHostPMFeature(bitmask, VIR_NODE_SUSPEND_TARGET_HYBRID); if (ret < 0) return -1; diff --git a/src/util/util.h b/src/util/util.h index eda60d2..690fca0 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -263,14 +263,6 @@ void virTypedParameterArrayClear(virTypedParameterPtr params, int nparams); /* Power Management Capabilities of the host system */ -enum virHostPMCapability { - VIR_HOST_PM_S3, /* Suspend-to-RAM */ - VIR_HOST_PM_S4, /* Suspend-to-Disk */ - VIR_HOST_PM_HYBRID_SUSPEND, /* Hybrid-Suspend */ - - VIR_HOST_PM_LAST -}; - VIR_ENUM_DECL(virHostPMCapability) int virDiscoverHostPMFeature(unsigned int *bitmask, unsigned int feature); -- 1.7.6.4
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The internal virHostPMCapability enum just duplicates the public virNodeSuspendTarget enum, but with different names.
* src/util/util.c: Use VIR_NODE_SUSPEND_TARGET constants * src/util/util.h: Remove virHostPMCapability enum * src/conf/capabilities.c: Use VIR_NODE_SUSPEND_TARGET_LAST --- src/conf/capabilities.c | 2 +- src/util/util.c | 18 +++++++++--------- src/util/util.h | 8 -------- 3 files changed, 10 insertions(+), 18 deletions(-)
+++ b/src/util/util.h @@ -263,14 +263,6 @@ void virTypedParameterArrayClear(virTypedParameterPtr params, int nparams);
/* Power Management Capabilities of the host system */
-enum virHostPMCapability { - VIR_HOST_PM_S3, /* Suspend-to-RAM */ - VIR_HOST_PM_S4, /* Suspend-to-Disk */ - VIR_HOST_PM_HYBRID_SUSPEND, /* Hybrid-Suspend */ - - VIR_HOST_PM_LAST -}; - VIR_ENUM_DECL(virHostPMCapability)
Also drop the VIR_ENUM_DECL(virHostPMCapability). ACK to the intent, but this is incomplete. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Nov 29, 2011 at 09:13:57AM -0700, Eric Blake wrote:
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The internal virHostPMCapability enum just duplicates the public virNodeSuspendTarget enum, but with different names.
* src/util/util.c: Use VIR_NODE_SUSPEND_TARGET constants * src/util/util.h: Remove virHostPMCapability enum * src/conf/capabilities.c: Use VIR_NODE_SUSPEND_TARGET_LAST --- src/conf/capabilities.c | 2 +- src/util/util.c | 18 +++++++++--------- src/util/util.h | 8 -------- 3 files changed, 10 insertions(+), 18 deletions(-)
+++ b/src/util/util.h @@ -263,14 +263,6 @@ void virTypedParameterArrayClear(virTypedParameterPtr params, int nparams);
/* Power Management Capabilities of the host system */
-enum virHostPMCapability { - VIR_HOST_PM_S3, /* Suspend-to-RAM */ - VIR_HOST_PM_S4, /* Suspend-to-Disk */ - VIR_HOST_PM_HYBRID_SUSPEND, /* Hybrid-Suspend */ - - VIR_HOST_PM_LAST -}; - VIR_ENUM_DECL(virHostPMCapability)
Also drop the VIR_ENUM_DECL(virHostPMCapability).
ACK to the intent, but this is incomplete.
See patch 4 :-) Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
From: "Daniel P. Berrange" <berrange@redhat.com> The capabilities XML uses the x86 specific terms 'S3', 'S4' and 'Hybrid-Syspend'. Switch it to use the same terminology as the API constants and virsh options, eg 'suspend_mem' 'suspend_disk' and 'syspend_hybrid' * docs/formatcaps.html.in, docs/schemas/capability.rng, src/conf/capabilities.c: Rename suspend constants --- docs/formatcaps.html.in | 6 +++--- docs/schemas/capability.rng | 6 +++--- src/conf/capabilities.c | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/docs/formatcaps.html.in b/docs/formatcaps.html.in index c1bc2f5..8792533 100644 --- a/docs/formatcaps.html.in +++ b/docs/formatcaps.html.in @@ -29,9 +29,9 @@ BIOS you will see</p> ... </cpu> <power_management> - <S3/> - <S4/> - <Hybrid-Suspend/> + <suspend-mem/> + <suspend-disk/> + <suspend-hybrid/> <power_management/> </host></span> diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng index 6cf2188..3af95e9 100644 --- a/docs/schemas/capability.rng +++ b/docs/schemas/capability.rng @@ -112,17 +112,17 @@ <element name='power_management'> <interleave> <optional> - <element name='S3'> + <element name='suspend_mem'> <empty/> </element> </optional> <optional> - <element name='S4'> + <element name='suspend_disk'> <empty/> </element> </optional> <optional> - <element name='Hybrid-Suspend'> + <element name='suspend_hybrid'> <empty/> </element> </optional> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index a2ca46b..ecb1dcd 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -37,7 +37,7 @@ #define VIR_FROM_THIS VIR_FROM_CAPABILITIES VIR_ENUM_IMPL(virHostPMCapability, VIR_NODE_SUSPEND_TARGET_LAST, - "S3", "S4", "Hybrid-Suspend") + "suspend_mem", "suspend_disk", "suspend_hybrid"); /** * virCapabilitiesNew: -- 1.7.6.4
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The capabilities XML uses the x86 specific terms 'S3', 'S4' and 'Hybrid-Syspend'. Switch it to use the same terminology as the API constants and virsh options, eg 'suspend_mem' 'suspend_disk' and 'syspend_hybrid'
s/syspend/suspend/
* docs/formatcaps.html.in, docs/schemas/capability.rng, src/conf/capabilities.c: Rename suspend constants --- docs/formatcaps.html.in | 6 +++--- docs/schemas/capability.rng | 6 +++--- src/conf/capabilities.c | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/docs/formatcaps.html.in b/docs/formatcaps.html.in index c1bc2f5..8792533 100644 --- a/docs/formatcaps.html.in +++ b/docs/formatcaps.html.in @@ -29,9 +29,9 @@ BIOS you will see</p> ... </cpu> <power_management> - <S3/> - <S4/> - <Hybrid-Suspend/> + <suspend-mem/> + <suspend-disk/> + <suspend-hybrid/> <power_management/>
3 typos - per the .rng, this should be suspend_mem, not suspend-mem, and so forth for the other '-'.
+++ b/src/conf/capabilities.c @@ -37,7 +37,7 @@ #define VIR_FROM_THIS VIR_FROM_CAPABILITIES
VIR_ENUM_IMPL(virHostPMCapability, VIR_NODE_SUSPEND_TARGET_LAST, - "S3", "S4", "Hybrid-Suspend") + "suspend_mem", "suspend_disk", "suspend_hybrid");
ACK with nit fixed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> The virHostPMCapability enum helper was declared in util.h but implemented in capabilities.c, which is in a completely separate library at link time. Move the declaration into the capabilities.c file and rename it to match normal conventions * src/util/util.h: Remove virHostPMCapability enum decl * src/conf/capabilities.c: Add virCapsHostPMTarget enm --- src/conf/capabilities.c | 5 +++-- src/util/util.h | 2 -- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index ecb1dcd..df5ff23 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -36,7 +36,8 @@ #define VIR_FROM_THIS VIR_FROM_CAPABILITIES -VIR_ENUM_IMPL(virHostPMCapability, VIR_NODE_SUSPEND_TARGET_LAST, +VIR_ENUM_DECL(virCapsHostPMTarget) +VIR_ENUM_IMPL(virCapsHostPMTarget, VIR_NODE_SUSPEND_TARGET_LAST, "suspend_mem", "suspend_disk", "suspend_hybrid"); /** @@ -704,7 +705,7 @@ virCapabilitiesFormatXML(virCapsPtr caps) while (pm) { int bit = ffs(pm) - 1; virBufferAsprintf(&xml, " <%s/>\n", - virHostPMCapabilityTypeToString(bit)); + virCapsHostPMTargetTypeToString(bit)); pm &= ~(1U << bit); } virBufferAddLit(&xml, " </power_management>\n"); diff --git a/src/util/util.h b/src/util/util.h index 690fca0..204e2b9 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -263,8 +263,6 @@ void virTypedParameterArrayClear(virTypedParameterPtr params, int nparams); /* Power Management Capabilities of the host system */ -VIR_ENUM_DECL(virHostPMCapability) - int virDiscoverHostPMFeature(unsigned int *bitmask, unsigned int feature); int virGetPMCapabilities(unsigned int *bitmask); -- 1.7.6.4
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The virHostPMCapability enum helper was declared in util.h but implemented in capabilities.c, which is in a completely separate library at link time. Move the declaration into the capabilities.c file and rename it to match normal conventions
* src/util/util.h: Remove virHostPMCapability enum decl * src/conf/capabilities.c: Add virCapsHostPMTarget enm
s/enm/enum/ ACK, and fixes my comment from patch 2/14 :) -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> Since virDiscoverHostPMFeature is just checking one feature, there is no reason for it to return a bitmask. Change it to return a boolean * src/util/util.c, src/util/util.h: Make virDiscoverHostPMFeature return a boolean --- src/util/util.c | 24 +++++++++++++++--------- src/util/util.h | 2 +- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 5bd8c91..badfa3a 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2624,13 +2624,12 @@ virTypedParameterArrayClear(virTypedParameterPtr params, int nparams) /** * virDiscoverHostPMFeature: - * @bitmask: The bitmask which should be populated with the result of - * the query * @feature: The power management feature to check whether it is supported * by the host. Values could be: * VIR_NODE_SUSPEND_TARGET_MEM * VIR_NODE_SUSPEND_TARGET_DISK * VIR_NODE_SUSPEND_TARGET_HYBRID + * @supported: set to true if supported, false otherwise * * Run the script 'pm-is-supported' (from the pm-utils package) * to find out if @feature is supported by the host. @@ -2638,12 +2637,14 @@ virTypedParameterArrayClear(virTypedParameterPtr params, int nparams) * Returns 0 if the query was successful, -1 on failure. */ int -virDiscoverHostPMFeature(unsigned int *bitmask, unsigned int feature) +virDiscoverHostPMFeature(unsigned int feature, bool *supported) { virCommandPtr cmd; int status; int ret = -1; + *supported = false; + switch (feature) { case VIR_NODE_SUSPEND_TARGET_MEM: cmd = virCommandNewArgList("pm-is-supported", "--suspend", NULL); @@ -2665,9 +2666,7 @@ virDiscoverHostPMFeature(unsigned int *bitmask, unsigned int feature) * Check return code of command == 0 for success * (i.e., the PM capability is supported) */ - if (status == 0) - *bitmask |= 1U << feature; - + *supported = (status == 0); ret = 0; cleanup: @@ -2691,23 +2690,30 @@ int virGetPMCapabilities(unsigned int *bitmask) { int ret; + bool supported; *bitmask = 0; /* Check support for Suspend-to-RAM (S3) */ - ret = virDiscoverHostPMFeature(bitmask, VIR_NODE_SUSPEND_TARGET_MEM); + ret = virDiscoverHostPMFeature(VIR_NODE_SUSPEND_TARGET_MEM, &supported); if (ret < 0) return -1; + if (supported) + *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_MEM); /* Check support for Suspend-to-Disk (S4) */ - ret = virDiscoverHostPMFeature(bitmask, VIR_NODE_SUSPEND_TARGET_DISK); + ret = virDiscoverHostPMFeature(VIR_NODE_SUSPEND_TARGET_DISK, &supported); if (ret < 0) return -1; + if (supported) + *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_DISK); /* Check support for Hybrid-Suspend */ - ret = virDiscoverHostPMFeature(bitmask, VIR_NODE_SUSPEND_TARGET_HYBRID); + ret = virDiscoverHostPMFeature(VIR_NODE_SUSPEND_TARGET_HYBRID, &supported); if (ret < 0) return -1; + if (supported) + *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_HYBRID); return 0; } diff --git a/src/util/util.h b/src/util/util.h index 204e2b9..6a9de7e 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -263,7 +263,7 @@ void virTypedParameterArrayClear(virTypedParameterPtr params, int nparams); /* Power Management Capabilities of the host system */ -int virDiscoverHostPMFeature(unsigned int *bitmask, unsigned int feature); +int virDiscoverHostPMFeature(unsigned int feature, bool *supported); int virGetPMCapabilities(unsigned int *bitmask); #endif /* __VIR_UTIL_H__ */ -- 1.7.6.4
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Since virDiscoverHostPMFeature is just checking one feature, there is no reason for it to return a bitmask. Change it to return a boolean
* src/util/util.c, src/util/util.h: Make virDiscoverHostPMFeature return a boolean --- src/util/util.c | 24 +++++++++++++++--------- src/util/util.h | 2 +- 2 files changed, 16 insertions(+), 10 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/29/2011 09:14 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
cleanup: @@ -2691,23 +2690,30 @@ int virGetPMCapabilities(unsigned int *bitmask) { int ret;
The variable 'ret' is unnecessary in this function. So we can probably remove it, to do further cleanup.
+ bool supported;
*bitmask = 0;
/* Check support for Suspend-to-RAM (S3) */ - ret = virDiscoverHostPMFeature(bitmask, VIR_NODE_SUSPEND_TARGET_MEM); + ret = virDiscoverHostPMFeature(VIR_NODE_SUSPEND_TARGET_MEM, &supported); if (ret < 0) return -1; + if (supported) + *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_MEM);
/* Check support for Suspend-to-Disk (S4) */ - ret = virDiscoverHostPMFeature(bitmask, VIR_NODE_SUSPEND_TARGET_DISK); + ret = virDiscoverHostPMFeature(VIR_NODE_SUSPEND_TARGET_DISK, &supported); if (ret < 0) return -1; + if (supported) + *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_DISK);
/* Check support for Hybrid-Suspend */ - ret = virDiscoverHostPMFeature(bitmask, VIR_NODE_SUSPEND_TARGET_HYBRID); + ret = virDiscoverHostPMFeature(VIR_NODE_SUSPEND_TARGET_HYBRID, &supported); if (ret < 0) return -1; + if (supported) + *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_HYBRID);
return 0; } diff --git a/src/util/util.h b/src/util/util.h index 204e2b9..6a9de7e 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -263,7 +263,7 @@ void virTypedParameterArrayClear(virTypedParameterPtr params, int nparams);
/* Power Management Capabilities of the host system */
-int virDiscoverHostPMFeature(unsigned int *bitmask, unsigned int feature); +int virDiscoverHostPMFeature(unsigned int feature, bool *supported); int virGetPMCapabilities(unsigned int *bitmask);
#endif /* __VIR_UTIL_H__ */
-- Regards, Srivatsa S. Bhat IBM Linux Technology Center
From: "Daniel P. Berrange" <berrange@redhat.com> Rename virGetPMCapabilities to virNodeSuspendGetTargetMask and virDiscoverHostPMFeature to virNodeSuspendSupportsTarget. * src/util/util.c, src/util/util.h: Rename APIs * src/qemu/qemu_capabilities.c, src/util/virnodesuspend.c: Adjust for new names --- src/qemu/qemu_capabilities.c | 2 +- src/util/util.c | 22 +++++++++++----------- src/util/util.h | 4 ++-- src/util/virnodesuspend.c | 2 +- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index c5fe41d..4bbfd78 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -850,7 +850,7 @@ virCapsPtr qemuCapsInit(virCapsPtr old_caps) /* Add the power management features of the host */ - if (virGetPMCapabilities(&caps->host.powerMgmt) < 0) { + if (virNodeSuspendGetTargetMask(&caps->host.powerMgmt) < 0) { VIR_WARN("Failed to get host power management capabilities"); caps->host.powerMgmt_valid = false; } else diff --git a/src/util/util.c b/src/util/util.c index badfa3a..72fbdac 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2623,8 +2623,8 @@ virTypedParameterArrayClear(virTypedParameterPtr params, int nparams) } /** - * virDiscoverHostPMFeature: - * @feature: The power management feature to check whether it is supported + * virNodeSuspendSupportsTarget: + * @target: The power management target to check whether it is supported * by the host. Values could be: * VIR_NODE_SUSPEND_TARGET_MEM * VIR_NODE_SUSPEND_TARGET_DISK @@ -2632,12 +2632,12 @@ virTypedParameterArrayClear(virTypedParameterPtr params, int nparams) * @supported: set to true if supported, false otherwise * * Run the script 'pm-is-supported' (from the pm-utils package) - * to find out if @feature is supported by the host. + * to find out if @target is supported by the host. * * Returns 0 if the query was successful, -1 on failure. */ int -virDiscoverHostPMFeature(unsigned int feature, bool *supported) +virNodeSuspendSupportsTarget(unsigned int target, bool *supported) { virCommandPtr cmd; int status; @@ -2645,7 +2645,7 @@ virDiscoverHostPMFeature(unsigned int feature, bool *supported) *supported = false; - switch (feature) { + switch (target) { case VIR_NODE_SUSPEND_TARGET_MEM: cmd = virCommandNewArgList("pm-is-supported", "--suspend", NULL); break; @@ -2675,19 +2675,19 @@ cleanup: } /** - * virGetPMCapabilities: + * virNodeSuspendGetTargetMask: * * Get the Power Management Capabilities that the host system supports, * such as Suspend-to-RAM (S3), Suspend-to-Disk (S4) and Hybrid-Suspend * (a combination of S3 and S4). * * @bitmask: Pointer to the bitmask which will be set appropriately to - * indicate all the supported host power management features. + * indicate all the supported host power management targets. * * Returns 0 if the query was successful, -1 on failure. */ int -virGetPMCapabilities(unsigned int *bitmask) +virNodeSuspendGetTargetMask(unsigned int *bitmask) { int ret; bool supported; @@ -2695,21 +2695,21 @@ virGetPMCapabilities(unsigned int *bitmask) *bitmask = 0; /* Check support for Suspend-to-RAM (S3) */ - ret = virDiscoverHostPMFeature(VIR_NODE_SUSPEND_TARGET_MEM, &supported); + ret = virNodeSuspendSupportsTarget(VIR_NODE_SUSPEND_TARGET_MEM, &supported); if (ret < 0) return -1; if (supported) *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_MEM); /* Check support for Suspend-to-Disk (S4) */ - ret = virDiscoverHostPMFeature(VIR_NODE_SUSPEND_TARGET_DISK, &supported); + ret = virNodeSuspendSupportsTarget(VIR_NODE_SUSPEND_TARGET_DISK, &supported); if (ret < 0) return -1; if (supported) *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_DISK); /* Check support for Hybrid-Suspend */ - ret = virDiscoverHostPMFeature(VIR_NODE_SUSPEND_TARGET_HYBRID, &supported); + ret = virNodeSuspendSupportsTarget(VIR_NODE_SUSPEND_TARGET_HYBRID, &supported); if (ret < 0) return -1; if (supported) diff --git a/src/util/util.h b/src/util/util.h index 6a9de7e..6713547 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -263,7 +263,7 @@ void virTypedParameterArrayClear(virTypedParameterPtr params, int nparams); /* Power Management Capabilities of the host system */ -int virDiscoverHostPMFeature(unsigned int feature, bool *supported); -int virGetPMCapabilities(unsigned int *bitmask); +int virNodeSuspendSupportsTarget(unsigned int target, bool *supported); +int virNodeSuspendGetTargetMask(unsigned int *bitmask); #endif /* __VIR_UTIL_H__ */ diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c index 01feaa2..0814c36 100644 --- a/src/util/virnodesuspend.c +++ b/src/util/virnodesuspend.c @@ -81,7 +81,7 @@ int virNodeSuspendInit(void) /* Get the power management capabilities supported by the host */ hostPMFeatures = 0; - if (virGetPMCapabilities(&hostPMFeatures) < 0) { + if (virNodeSuspendGetTargetMask(&hostPMFeatures) < 0) { if (geteuid() == 0) VIR_ERROR(_("Failed to get host power management features")); } -- 1.7.6.4
On 11/29/2011 09:14 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Rename virGetPMCapabilities to virNodeSuspendGetTargetMask and virDiscoverHostPMFeature to virNodeSuspendSupportsTarget.
* src/util/util.c, src/util/util.h: Rename APIs * src/qemu/qemu_capabilities.c, src/util/virnodesuspend.c: Adjust for new names --- src/qemu/qemu_capabilities.c | 2 +- src/util/util.c | 22 +++++++++++----------- src/util/util.h | 4 ++-- src/util/virnodesuspend.c | 2 +- 4 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index c5fe41d..4bbfd78 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -850,7 +850,7 @@ virCapsPtr qemuCapsInit(virCapsPtr old_caps)
/* Add the power management features of the host */
- if (virGetPMCapabilities(&caps->host.powerMgmt) < 0) { + if (virNodeSuspendGetTargetMask(&caps->host.powerMgmt) < 0) { VIR_WARN("Failed to get host power management capabilities"); caps->host.powerMgmt_valid = false; } else diff --git a/src/util/util.c b/src/util/util.c index badfa3a..72fbdac 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2623,8 +2623,8 @@ virTypedParameterArrayClear(virTypedParameterPtr params, int nparams) }
/** - * virDiscoverHostPMFeature: - * @feature: The power management feature to check whether it is supported + * virNodeSuspendSupportsTarget: + * @target: The power management target to check whether it is supported * by the host. Values could be: * VIR_NODE_SUSPEND_TARGET_MEM * VIR_NODE_SUSPEND_TARGET_DISK @@ -2632,12 +2632,12 @@ virTypedParameterArrayClear(virTypedParameterPtr params, int nparams) * @supported: set to true if supported, false otherwise * * Run the script 'pm-is-supported' (from the pm-utils package) - * to find out if @feature is supported by the host. + * to find out if @target is supported by the host. * * Returns 0 if the query was successful, -1 on failure. */ int -virDiscoverHostPMFeature(unsigned int feature, bool *supported) +virNodeSuspendSupportsTarget(unsigned int target, bool *supported) { virCommandPtr cmd; int status; @@ -2645,7 +2645,7 @@ virDiscoverHostPMFeature(unsigned int feature, bool *supported)
*supported = false;
- switch (feature) { + switch (target) { case VIR_NODE_SUSPEND_TARGET_MEM: cmd = virCommandNewArgList("pm-is-supported", "--suspend", NULL); break; @@ -2675,19 +2675,19 @@ cleanup: }
/** - * virGetPMCapabilities: + * virNodeSuspendGetTargetMask: * * Get the Power Management Capabilities that the host system supports, * such as Suspend-to-RAM (S3), Suspend-to-Disk (S4) and Hybrid-Suspend * (a combination of S3 and S4). *
You might want to remove the terminology "S3", "S4" etc from here, perhaps? -- Regards, Srivatsa S. Bhat IBM Linux Technology Center
On 11/29/2011 09:19 AM, Srivatsa S. Bhat wrote:
/** - * virGetPMCapabilities: + * virNodeSuspendGetTargetMask: * * Get the Power Management Capabilities that the host system supports, * such as Suspend-to-RAM (S3), Suspend-to-Disk (S4) and Hybrid-Suspend * (a combination of S3 and S4). *
You might want to remove the terminology "S3", "S4" etc from here, perhaps?
Nah. While we don't want the public XML or enum names to reflect an x86-specific naming convention, there's nothing wrong with internal APIs using terminology in comments to make it easier to correlate the generic name with the specific feature, so that developers familiar with either term can be sure to understand. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/29/2011 09:53 PM, Eric Blake wrote:
On 11/29/2011 09:19 AM, Srivatsa S. Bhat wrote:
/** - * virGetPMCapabilities: + * virNodeSuspendGetTargetMask: * * Get the Power Management Capabilities that the host system supports, * such as Suspend-to-RAM (S3), Suspend-to-Disk (S4) and Hybrid-Suspend * (a combination of S3 and S4). *
You might want to remove the terminology "S3", "S4" etc from here, perhaps?
Nah. While we don't want the public XML or enum names to reflect an x86-specific naming convention, there's nothing wrong with internal APIs using terminology in comments to make it easier to correlate the generic name with the specific feature, so that developers familiar with either term can be sure to understand.
Ok, that sounds good. So lets retain it then. -- Regards, Srivatsa S. Bhat IBM Linux Technology Center
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Rename virGetPMCapabilities to virNodeSuspendGetTargetMask and virDiscoverHostPMFeature to virNodeSuspendSupportsTarget.
Makes sense for more consistent naming. ACK for mechanical rename. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> The node suspend capabilities APIs should not have been put into util.[ch]. Instead move them into virnodesuspend.[ch] * src/util/util.c, src/util/util.h: Remove suspend capabilities APIs * src/util/virnodesuspend.c, src/util/virnodesuspend.h: Add suspend capabilities APIs * src/qemu/qemu_capabilities.c: Include virnodesuspend.h --- src/qemu/qemu_capabilities.c | 1 + src/util/util.c | 96 ----------------------------------------- src/util/util.h | 5 -- src/util/virnodesuspend.c | 97 ++++++++++++++++++++++++++++++++++++++++++ src/util/virnodesuspend.h | 2 + 5 files changed, 100 insertions(+), 101 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 4bbfd78..64ab8a8 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -34,6 +34,7 @@ #include "domain_conf.h" #include "qemu_conf.h" #include "command.h" +#include "virnodesuspend.h" #include <sys/stat.h> #include <unistd.h> diff --git a/src/util/util.c b/src/util/util.c index 72fbdac..9ecfa9d 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2621,99 +2621,3 @@ virTypedParameterArrayClear(virTypedParameterPtr params, int nparams) VIR_FREE(params[i].value.s); } } - -/** - * virNodeSuspendSupportsTarget: - * @target: The power management target to check whether it is supported - * by the host. Values could be: - * VIR_NODE_SUSPEND_TARGET_MEM - * VIR_NODE_SUSPEND_TARGET_DISK - * VIR_NODE_SUSPEND_TARGET_HYBRID - * @supported: set to true if supported, false otherwise - * - * Run the script 'pm-is-supported' (from the pm-utils package) - * to find out if @target is supported by the host. - * - * Returns 0 if the query was successful, -1 on failure. - */ -int -virNodeSuspendSupportsTarget(unsigned int target, bool *supported) -{ - virCommandPtr cmd; - int status; - int ret = -1; - - *supported = false; - - switch (target) { - case VIR_NODE_SUSPEND_TARGET_MEM: - cmd = virCommandNewArgList("pm-is-supported", "--suspend", NULL); - break; - case VIR_NODE_SUSPEND_TARGET_DISK: - cmd = virCommandNewArgList("pm-is-supported", "--hibernate", NULL); - break; - case VIR_NODE_SUSPEND_TARGET_HYBRID: - cmd = virCommandNewArgList("pm-is-supported", "--suspend-hybrid", NULL); - break; - default: - return ret; - } - - if (virCommandRun(cmd, &status) < 0) - goto cleanup; - - /* - * Check return code of command == 0 for success - * (i.e., the PM capability is supported) - */ - *supported = (status == 0); - ret = 0; - -cleanup: - virCommandFree(cmd); - return ret; -} - -/** - * virNodeSuspendGetTargetMask: - * - * Get the Power Management Capabilities that the host system supports, - * such as Suspend-to-RAM (S3), Suspend-to-Disk (S4) and Hybrid-Suspend - * (a combination of S3 and S4). - * - * @bitmask: Pointer to the bitmask which will be set appropriately to - * indicate all the supported host power management targets. - * - * Returns 0 if the query was successful, -1 on failure. - */ -int -virNodeSuspendGetTargetMask(unsigned int *bitmask) -{ - int ret; - bool supported; - - *bitmask = 0; - - /* Check support for Suspend-to-RAM (S3) */ - ret = virNodeSuspendSupportsTarget(VIR_NODE_SUSPEND_TARGET_MEM, &supported); - if (ret < 0) - return -1; - if (supported) - *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_MEM); - - /* Check support for Suspend-to-Disk (S4) */ - ret = virNodeSuspendSupportsTarget(VIR_NODE_SUSPEND_TARGET_DISK, &supported); - if (ret < 0) - return -1; - if (supported) - *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_DISK); - - /* Check support for Hybrid-Suspend */ - ret = virNodeSuspendSupportsTarget(VIR_NODE_SUSPEND_TARGET_HYBRID, &supported); - if (ret < 0) - return -1; - if (supported) - *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_HYBRID); - - return 0; -} diff --git a/src/util/util.h b/src/util/util.h index 6713547..ee53b84 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -261,9 +261,4 @@ int virEmitXMLWarning(int fd, void virTypedParameterArrayClear(virTypedParameterPtr params, int nparams); -/* Power Management Capabilities of the host system */ - -int virNodeSuspendSupportsTarget(unsigned int target, bool *supported); -int virNodeSuspendGetTargetMask(unsigned int *bitmask); - #endif /* __VIR_UTIL_H__ */ diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c index 0814c36..4ac0d45 100644 --- a/src/util/virnodesuspend.c +++ b/src/util/virnodesuspend.c @@ -269,3 +269,100 @@ cleanup: VIR_FREE(cmdString); return -1; } + + +/** + * virNodeSuspendSupportsTarget: + * @target: The power management target to check whether it is supported + * by the host. Values could be: + * VIR_NODE_SUSPEND_TARGET_MEM + * VIR_NODE_SUSPEND_TARGET_DISK + * VIR_NODE_SUSPEND_TARGET_HYBRID + * @supported: set to true if supported, false otherwise + * + * Run the script 'pm-is-supported' (from the pm-utils package) + * to find out if @target is supported by the host. + * + * Returns 0 if the query was successful, -1 on failure. + */ +int +virNodeSuspendSupportsTarget(unsigned int target, bool *supported) +{ + virCommandPtr cmd; + int status; + int ret = -1; + + *supported = false; + + switch (target) { + case VIR_NODE_SUSPEND_TARGET_MEM: + cmd = virCommandNewArgList("pm-is-supported", "--suspend", NULL); + break; + case VIR_NODE_SUSPEND_TARGET_DISK: + cmd = virCommandNewArgList("pm-is-supported", "--hibernate", NULL); + break; + case VIR_NODE_SUSPEND_TARGET_HYBRID: + cmd = virCommandNewArgList("pm-is-supported", "--suspend-hybrid", NULL); + break; + default: + return ret; + } + + if (virCommandRun(cmd, &status) < 0) + goto cleanup; + + /* + * Check return code of command == 0 for success + * (i.e., the PM capability is supported) + */ + *supported = (status == 0); + ret = 0; + +cleanup: + virCommandFree(cmd); + return ret; +} + +/** + * virNodeSuspendGetTargetMask: + * + * Get the Power Management Capabilities that the host system supports, + * such as Suspend-to-RAM (S3), Suspend-to-Disk (S4) and Hybrid-Suspend + * (a combination of S3 and S4). + * + * @bitmask: Pointer to the bitmask which will be set appropriately to + * indicate all the supported host power management targets. + * + * Returns 0 if the query was successful, -1 on failure. + */ +int +virNodeSuspendGetTargetMask(unsigned int *bitmask) +{ + int ret; + bool supported; + + *bitmask = 0; + + /* Check support for Suspend-to-RAM (S3) */ + ret = virNodeSuspendSupportsTarget(VIR_NODE_SUSPEND_TARGET_MEM, &supported); + if (ret < 0) + return -1; + if (supported) + *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_MEM); + + /* Check support for Suspend-to-Disk (S4) */ + ret = virNodeSuspendSupportsTarget(VIR_NODE_SUSPEND_TARGET_DISK, &supported); + if (ret < 0) + return -1; + if (supported) + *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_DISK); + + /* Check support for Hybrid-Suspend */ + ret = virNodeSuspendSupportsTarget(VIR_NODE_SUSPEND_TARGET_HYBRID, &supported); + if (ret < 0) + return -1; + if (supported) + *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_HYBRID); + + return 0; +} diff --git a/src/util/virnodesuspend.h b/src/util/virnodesuspend.h index 66e3214..1e23ce8 100644 --- a/src/util/virnodesuspend.h +++ b/src/util/virnodesuspend.h @@ -32,5 +32,7 @@ int nodeSuspendForDuration(virConnectPtr conn, int virNodeSuspendInit(void); +int virNodeSuspendSupportsTarget(unsigned int target, bool *supported); +int virNodeSuspendGetTargetMask(unsigned int *bitmask); #endif /* __VIR_NODE_SUSPEND_H__ */ -- 1.7.6.4
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The node suspend capabilities APIs should not have been put into util.[ch]. Instead move them into virnodesuspend.[ch]
* src/util/util.c, src/util/util.h: Remove suspend capabilities APIs * src/util/virnodesuspend.c, src/util/virnodesuspend.h: Add suspend capabilities APIs * src/qemu/qemu_capabilities.c: Include virnodesuspend.h --- src/qemu/qemu_capabilities.c | 1 + src/util/util.c | 96 ----------------------------------------- src/util/util.h | 5 -- src/util/virnodesuspend.c | 97 ++++++++++++++++++++++++++++++++++++++++++ src/util/virnodesuspend.h | 2 + 5 files changed, 100 insertions(+), 101 deletions(-)
This should also be updating the list of functions in the virnodesuspend.h section of libvirt_private.syms. ACK with that tweak. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/29/2011 09:14 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
+ + +/** + * virNodeSuspendSupportsTarget: + * @target: The power management target to check whether it is supported + * by the host. Values could be: + * VIR_NODE_SUSPEND_TARGET_MEM + * VIR_NODE_SUSPEND_TARGET_DISK + * VIR_NODE_SUSPEND_TARGET_HYBRID + * @supported: set to true if supported, false otherwise + * + * Run the script 'pm-is-supported' (from the pm-utils package) + * to find out if @target is supported by the host. + * + * Returns 0 if the query was successful, -1 on failure. + */ +int +virNodeSuspendSupportsTarget(unsigned int target, bool *supported) +{
We can mark this function as static, since it is only ever called by virNodeSuspendGetTargetMask().
+ virCommandPtr cmd; + int status; + int ret = -1; + + *supported = false; + + switch (target) { + case VIR_NODE_SUSPEND_TARGET_MEM: + cmd = virCommandNewArgList("pm-is-supported", "--suspend", NULL); + break; + case VIR_NODE_SUSPEND_TARGET_DISK: + cmd = virCommandNewArgList("pm-is-supported", "--hibernate", NULL); + break; + case VIR_NODE_SUSPEND_TARGET_HYBRID: + cmd = virCommandNewArgList("pm-is-supported", "--suspend-hybrid", NULL); + break; + default: + return ret; + } + + if (virCommandRun(cmd, &status) < 0) + goto cleanup; + + /* + * Check return code of command == 0 for success + * (i.e., the PM capability is supported) + */ + *supported = (status == 0); + ret = 0; + +cleanup: + virCommandFree(cmd); + return ret; +} + +/** + * virNodeSuspendGetTargetMask: + * + * Get the Power Management Capabilities that the host system supports, + * such as Suspend-to-RAM (S3), Suspend-to-Disk (S4) and Hybrid-Suspend + * (a combination of S3 and S4). + * + * @bitmask: Pointer to the bitmask which will be set appropriately to + * indicate all the supported host power management targets. + * + * Returns 0 if the query was successful, -1 on failure. + */ +int +virNodeSuspendGetTargetMask(unsigned int *bitmask) +{ + int ret; + bool supported; + + *bitmask = 0; + + /* Check support for Suspend-to-RAM (S3) */ + ret = virNodeSuspendSupportsTarget(VIR_NODE_SUSPEND_TARGET_MEM, &supported); + if (ret < 0) + return -1; + if (supported) + *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_MEM); + + /* Check support for Suspend-to-Disk (S4) */ + ret = virNodeSuspendSupportsTarget(VIR_NODE_SUSPEND_TARGET_DISK, &supported); + if (ret < 0) + return -1; + if (supported) + *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_DISK); + + /* Check support for Hybrid-Suspend */ + ret = virNodeSuspendSupportsTarget(VIR_NODE_SUSPEND_TARGET_HYBRID, &supported); + if (ret < 0) + return -1; + if (supported) + *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_HYBRID); + + return 0; +} diff --git a/src/util/virnodesuspend.h b/src/util/virnodesuspend.h index 66e3214..1e23ce8 100644 --- a/src/util/virnodesuspend.h +++ b/src/util/virnodesuspend.h @@ -32,5 +32,7 @@ int nodeSuspendForDuration(virConnectPtr conn,
int virNodeSuspendInit(void);
+int virNodeSuspendSupportsTarget(unsigned int target, bool *supported); +int virNodeSuspendGetTargetMask(unsigned int *bitmask);
#endif /* __VIR_NODE_SUSPEND_H__ */
-- Regards, Srivatsa S. Bhat IBM Linux Technology Center
On Tue, Nov 29, 2011 at 11:31:30PM +0530, Srivatsa S. Bhat wrote:
On 11/29/2011 09:14 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
+ + +/** + * virNodeSuspendSupportsTarget: + * @target: The power management target to check whether it is supported + * by the host. Values could be: + * VIR_NODE_SUSPEND_TARGET_MEM + * VIR_NODE_SUSPEND_TARGET_DISK + * VIR_NODE_SUSPEND_TARGET_HYBRID + * @supported: set to true if supported, false otherwise + * + * Run the script 'pm-is-supported' (from the pm-utils package) + * to find out if @target is supported by the host. + * + * Returns 0 if the query was successful, -1 on failure. + */ +int +virNodeSuspendSupportsTarget(unsigned int target, bool *supported) +{
We can mark this function as static, since it is only ever called by virNodeSuspendGetTargetMask().
Yes, good point. Will make that change Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
From: "Daniel P. Berrange" <berrange@redhat.com> * src/libvirt_private.syms: Export virNodeSuspendSupportsTarget and virNodeSuspendGetTargetMask --- src/libvirt_private.syms | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9f2a224..a0754f2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1332,6 +1332,8 @@ virNetTLSContextNewServerPath; # virnodesuspend.h nodeSuspendForDuration; virNodeSuspendInit; +virNodeSuspendGetTargetMask; +virNodeSuspendSupportsTarget; # virpidfile.h -- 1.7.6.4
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
* src/libvirt_private.syms: Export virNodeSuspendSupportsTarget and virNodeSuspendGetTargetMask --- src/libvirt_private.syms | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9f2a224..a0754f2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1332,6 +1332,8 @@ virNetTLSContextNewServerPath; # virnodesuspend.h nodeSuspendForDuration; virNodeSuspendInit; +virNodeSuspendGetTargetMask; +virNodeSuspendSupportsTarget;
Aha - this covers my comment from patch 7/14. ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/29/2011 09:14 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
* src/libvirt_private.syms: Export virNodeSuspendSupportsTarget and virNodeSuspendGetTargetMask --- src/libvirt_private.syms | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9f2a224..a0754f2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1332,6 +1332,8 @@ virNetTLSContextNewServerPath; # virnodesuspend.h nodeSuspendForDuration; virNodeSuspendInit; +virNodeSuspendGetTargetMask; +virNodeSuspendSupportsTarget;
And we need not export virNodeSuspendSupportsTarget here, if we mark it as static in patch 7/14. -- Regards, Srivatsa S. Bhat IBM Linux Technology Center
From: "Daniel P. Berrange" <berrange@redhat.com> hostPMFeatures is a bitmask, but the VIR_NODE_SUSPEND_TARGET constants are from an enum. Thus the code was checking the wrong bit values * src/util/virnodesuspend.c: Fix suspend target checks --- src/util/virnodesuspend.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c index 4ac0d45..4eb5439 100644 --- a/src/util/virnodesuspend.c +++ b/src/util/virnodesuspend.c @@ -213,7 +213,7 @@ int nodeSuspendForDuration(virConnectPtr conn ATTRIBUTE_UNUSED, /* Check if the host supports the requested suspend target */ switch (target) { case VIR_NODE_SUSPEND_TARGET_MEM: - if (hostPMFeatures & VIR_NODE_SUSPEND_TARGET_MEM) { + if (hostPMFeatures & (1 << VIR_NODE_SUSPEND_TARGET_MEM)) { cmdString = strdup("pm-suspend"); if (cmdString == NULL) { virReportOOMError(); @@ -225,7 +225,7 @@ int nodeSuspendForDuration(virConnectPtr conn ATTRIBUTE_UNUSED, goto cleanup; case VIR_NODE_SUSPEND_TARGET_DISK: - if (hostPMFeatures & VIR_NODE_SUSPEND_TARGET_DISK) { + if (hostPMFeatures & (1 << VIR_NODE_SUSPEND_TARGET_DISK)) { cmdString = strdup("pm-hibernate"); if (cmdString == NULL) { virReportOOMError(); @@ -237,7 +237,7 @@ int nodeSuspendForDuration(virConnectPtr conn ATTRIBUTE_UNUSED, goto cleanup; case VIR_NODE_SUSPEND_TARGET_HYBRID: - if (hostPMFeatures & VIR_NODE_SUSPEND_TARGET_HYBRID) { + if (hostPMFeatures & (1 << VIR_NODE_SUSPEND_TARGET_HYBRID)) { cmdString = strdup("pm-suspend-hybrid"); if (cmdString == NULL) { virReportOOMError(); -- 1.7.6.4
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
hostPMFeatures is a bitmask, but the VIR_NODE_SUSPEND_TARGET constants are from an enum. Thus the code was checking the wrong bit values
Yep - regression introduced in patch 1/14. Maybe you should squash this in to that patch, so that bisecting doesn't hit the regression (although that would mean patching a different file, since you moved the function location in the meantime). Up to you if you want to go to the hassle. ACK whether rebased and squashed, or done here as a separate patch. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Nov 29, 2011 at 09:33:09AM -0700, Eric Blake wrote:
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
hostPMFeatures is a bitmask, but the VIR_NODE_SUSPEND_TARGET constants are from an enum. Thus the code was checking the wrong bit values
Yep - regression introduced in patch 1/14. Maybe you should squash this in to that patch, so that bisecting doesn't hit the regression (although that would mean patching a different file, since you moved the function location in the meantime). Up to you if you want to go to the hassle.
It wasn't a regression actually. This code was still using the duplicate enum at that point: enum virHostPMCapability { VIR_HOST_PM_S3, /* Suspend-to-RAM */ VIR_HOST_PM_S4, /* Suspend-to-Disk */ VIR_HOST_PM_HYBRID_SUSPEND, /* Hybrid-Suspend */ VIR_HOST_PM_LAST }; So AFAICT, it was broken from the start. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 11/29/2011 10:10 PM, Daniel P. Berrange wrote:
On Tue, Nov 29, 2011 at 09:33:09AM -0700, Eric Blake wrote:
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
hostPMFeatures is a bitmask, but the VIR_NODE_SUSPEND_TARGET constants are from an enum. Thus the code was checking the wrong bit values
Yep - regression introduced in patch 1/14. Maybe you should squash this in to that patch, so that bisecting doesn't hit the regression (although that would mean patching a different file, since you moved the function location in the meantime). Up to you if you want to go to the hassle.
It wasn't a regression actually. This code was still using the duplicate enum at that point:
enum virHostPMCapability { VIR_HOST_PM_S3, /* Suspend-to-RAM */ VIR_HOST_PM_S4, /* Suspend-to-Disk */ VIR_HOST_PM_HYBRID_SUSPEND, /* Hybrid-Suspend */
VIR_HOST_PM_LAST };
So AFAICT, it was broken from the start.
I beg to differ here. It was a bit weird, but definitely not broken. Since VIR_HOST_PM_S3 etc were linear (0,1,2), a bit-wise OR operation was carried out like this: *bitmask |= 1U << feature; where feature was one of the above. And while checking this, we used to do: hostPMFeatures & VIR_NODE_SUSPEND_TARGET_HYBRID where VIR_NODE_SUSPEND_TARGET_HYBRID etc were like 1 << 0, 1 << 1 etc Thus, this was ugly, but not broken. Otherwise, it would have surely failed my suspend tests which I ran every time before submitting the patches :-) -- Regards, Srivatsa S. Bhat IBM Linux Technology Center
On Tue, Nov 29, 2011 at 10:23:25PM +0530, Srivatsa S. Bhat wrote:
On 11/29/2011 10:10 PM, Daniel P. Berrange wrote:
On Tue, Nov 29, 2011 at 09:33:09AM -0700, Eric Blake wrote:
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
hostPMFeatures is a bitmask, but the VIR_NODE_SUSPEND_TARGET constants are from an enum. Thus the code was checking the wrong bit values
Yep - regression introduced in patch 1/14. Maybe you should squash this in to that patch, so that bisecting doesn't hit the regression (although that would mean patching a different file, since you moved the function location in the meantime). Up to you if you want to go to the hassle.
It wasn't a regression actually. This code was still using the duplicate enum at that point:
enum virHostPMCapability { VIR_HOST_PM_S3, /* Suspend-to-RAM */ VIR_HOST_PM_S4, /* Suspend-to-Disk */ VIR_HOST_PM_HYBRID_SUSPEND, /* Hybrid-Suspend */
VIR_HOST_PM_LAST };
So AFAICT, it was broken from the start.
I beg to differ here. It was a bit weird, but definitely not broken. Since VIR_HOST_PM_S3 etc were linear (0,1,2), a bit-wise OR operation was carried out like this: *bitmask |= 1U << feature; where feature was one of the above.
And while checking this, we used to do: hostPMFeatures & VIR_NODE_SUSPEND_TARGET_HYBRID
where VIR_NODE_SUSPEND_TARGET_HYBRID etc were like 1 << 0, 1 << 1 etc
Thus, this was ugly, but not broken. Otherwise, it would have surely failed my suspend tests which I ran every time before submitting the patches :-)
Actually yes you are right. I was confusing two bits of code Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
From: "Daniel P. Berrange" <berrange@redhat.com> If we ensure that virNodeSuspendGetTargetMask always resets *bitmask to zero upon failure, there is no need for the powerMgmt_valid field. * src/util/virnodesuspend.c: Ensure *bitmask is zero upon failure * src/conf/capabilities.c, src/conf/capabilities.h: Remove powerMgmt_valid field * src/qemu/qemu_capabilities.c: Remove powerMgmt_valid --- src/conf/capabilities.c | 30 ++++++++++++++---------------- src/conf/capabilities.h | 1 - src/qemu/qemu_capabilities.c | 5 +---- src/util/virnodesuspend.c | 10 +++++++--- 4 files changed, 22 insertions(+), 24 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index df5ff23..ac050df 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -696,23 +696,21 @@ virCapabilitiesFormatXML(virCapsPtr caps) virBufferAddLit(&xml, " </cpu>\n"); - if (caps->host.powerMgmt_valid) { - /* The PM query was successful. */ - if (caps->host.powerMgmt) { - /* The host supports some PM features. */ - unsigned int pm = caps->host.powerMgmt; - virBufferAddLit(&xml, " <power_management>\n"); - while (pm) { - int bit = ffs(pm) - 1; - virBufferAsprintf(&xml, " <%s/>\n", - virCapsHostPMTargetTypeToString(bit)); - pm &= ~(1U << bit); - } - virBufferAddLit(&xml, " </power_management>\n"); - } else { - /* The host does not support any PM feature. */ - virBufferAddLit(&xml, " <power_management/>\n"); + /* The PM query was successful. */ + if (caps->host.powerMgmt) { + /* The host supports some PM features. */ + unsigned int pm = caps->host.powerMgmt; + virBufferAddLit(&xml, " <power_management>\n"); + while (pm) { + int bit = ffs(pm) - 1; + virBufferAsprintf(&xml, " <%s/>\n", + virCapsHostPMTargetTypeToString(bit)); + pm &= ~(1U << bit); } + virBufferAddLit(&xml, " </power_management>\n"); + } else { + /* The host does not support any PM feature. */ + virBufferAddLit(&xml, " <power_management/>\n"); } if (caps->host.offlineMigrate) { diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index 148c7cc..7f35c17 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -105,7 +105,6 @@ struct _virCapsHost { size_t nfeatures; size_t nfeatures_max; char **features; - bool powerMgmt_valid; unsigned int powerMgmt; /* Bitmask of the PM capabilities. * See enum virHostPMCapability. */ diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 64ab8a8..deef0ea 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -851,11 +851,8 @@ virCapsPtr qemuCapsInit(virCapsPtr old_caps) /* Add the power management features of the host */ - if (virNodeSuspendGetTargetMask(&caps->host.powerMgmt) < 0) { + if (virNodeSuspendGetTargetMask(&caps->host.powerMgmt) < 0) VIR_WARN("Failed to get host power management capabilities"); - caps->host.powerMgmt_valid = false; - } else - caps->host.powerMgmt_valid = true; /* The PM query succeeded. */ virCapabilitiesAddHostMigrateTransport(caps, "tcp"); diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c index 4eb5439..6420f5b 100644 --- a/src/util/virnodesuspend.c +++ b/src/util/virnodesuspend.c @@ -346,23 +346,27 @@ virNodeSuspendGetTargetMask(unsigned int *bitmask) /* Check support for Suspend-to-RAM (S3) */ ret = virNodeSuspendSupportsTarget(VIR_NODE_SUSPEND_TARGET_MEM, &supported); if (ret < 0) - return -1; + goto error; if (supported) *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_MEM); /* Check support for Suspend-to-Disk (S4) */ ret = virNodeSuspendSupportsTarget(VIR_NODE_SUSPEND_TARGET_DISK, &supported); if (ret < 0) - return -1; + goto error; if (supported) *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_DISK); /* Check support for Hybrid-Suspend */ ret = virNodeSuspendSupportsTarget(VIR_NODE_SUSPEND_TARGET_HYBRID, &supported); if (ret < 0) - return -1; + goto error; if (supported) *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_HYBRID); return 0; + +error: + *bitmask = 0; + return -1; } -- 1.7.6.4
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
If we ensure that virNodeSuspendGetTargetMask always resets *bitmask to zero upon failure, there is no need for the powerMgmt_valid field.
* src/util/virnodesuspend.c: Ensure *bitmask is zero upon failure * src/conf/capabilities.c, src/conf/capabilities.h: Remove powerMgmt_valid field * src/qemu/qemu_capabilities.c: Remove powerMgmt_valid
I'm not sure about this one. This is a semantic change in the resulting XML.
- if (caps->host.powerMgmt_valid) { - /* The PM query was successful. */ - if (caps->host.powerMgmt) { - /* The host supports some PM features. */ - unsigned int pm = caps->host.powerMgmt; - virBufferAddLit(&xml, " <power_management>\n"); - while (pm) { - int bit = ffs(pm) - 1; - virBufferAsprintf(&xml, " <%s/>\n", - virCapsHostPMTargetTypeToString(bit)); - pm &= ~(1U << bit); - } - virBufferAddLit(&xml, " </power_management>\n"); - } else { - /* The host does not support any PM feature. */ - virBufferAddLit(&xml, " <power_management/>\n");
Before, we had three cases (and documented them!): no <power_management> in the XML - we could not query (or older server) explicit <power_management/> - we successfully queried, but lack power management explicit <power_management> with sub-elements - what features are supported
+ /* The PM query was successful. */ + if (caps->host.powerMgmt) { + /* The host supports some PM features. */ + unsigned int pm = caps->host.powerMgmt; + virBufferAddLit(&xml, " <power_management>\n"); + while (pm) { + int bit = ffs(pm) - 1; + virBufferAsprintf(&xml, " <%s/>\n", + virCapsHostPMTargetTypeToString(bit)); + pm &= ~(1U << bit); } + virBufferAddLit(&xml, " </power_management>\n"); + } else { + /* The host does not support any PM feature. */ + virBufferAddLit(&xml, " <power_management/>\n");
Whereas after the patch, we now _always_ output a form of <power_management>, thus collapsing 3 states into 2. On the other hand, what is a user going to do about the difference between the first two? There's nothing useful they can do by knowing whether a host lacks power management or whether libvirt lacked the ability to query power management; either way, it means they can't use power management API. So I'm 70-30 in favor of this patch. At any rate, ACK to the code, but you also need to update docs/formatcaps.html.in to match, if we agree to go with it. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Nov 29, 2011 at 09:40:14AM -0700, Eric Blake wrote:
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
If we ensure that virNodeSuspendGetTargetMask always resets *bitmask to zero upon failure, there is no need for the powerMgmt_valid field.
* src/util/virnodesuspend.c: Ensure *bitmask is zero upon failure * src/conf/capabilities.c, src/conf/capabilities.h: Remove powerMgmt_valid field * src/qemu/qemu_capabilities.c: Remove powerMgmt_valid
I'm not sure about this one. This is a semantic change in the resulting XML.
- if (caps->host.powerMgmt_valid) { - /* The PM query was successful. */ - if (caps->host.powerMgmt) { - /* The host supports some PM features. */ - unsigned int pm = caps->host.powerMgmt; - virBufferAddLit(&xml, " <power_management>\n"); - while (pm) { - int bit = ffs(pm) - 1; - virBufferAsprintf(&xml, " <%s/>\n", - virCapsHostPMTargetTypeToString(bit)); - pm &= ~(1U << bit); - } - virBufferAddLit(&xml, " </power_management>\n"); - } else { - /* The host does not support any PM feature. */ - virBufferAddLit(&xml, " <power_management/>\n");
Before, we had three cases (and documented them!):
no <power_management> in the XML - we could not query (or older server)
explicit <power_management/> - we successfully queried, but lack power management
explicit <power_management> with sub-elements - what features are supported
+ /* The PM query was successful. */ + if (caps->host.powerMgmt) { + /* The host supports some PM features. */ + unsigned int pm = caps->host.powerMgmt; + virBufferAddLit(&xml, " <power_management>\n"); + while (pm) { + int bit = ffs(pm) - 1; + virBufferAsprintf(&xml, " <%s/>\n", + virCapsHostPMTargetTypeToString(bit)); + pm &= ~(1U << bit); } + virBufferAddLit(&xml, " </power_management>\n"); + } else { + /* The host does not support any PM feature. */ + virBufferAddLit(&xml, " <power_management/>\n");
Whereas after the patch, we now _always_ output a form of <power_management>, thus collapsing 3 states into 2.
On the other hand, what is a user going to do about the difference between the first two? There's nothing useful they can do by knowing whether a host lacks power management or whether libvirt lacked the ability to query power management; either way, it means they can't use power management API.
As you say its not really useful information, and IMHO we shouldn't be exposing whether some internal operation failed or not, in the XML schema.
So I'm 70-30 in favor of this patch.
At any rate, ACK to the code, but you also need to update docs/formatcaps.html.in to match, if we agree to go with it.
OK will update the docs Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 11/29/2011 09:14 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
If we ensure that virNodeSuspendGetTargetMask always resets *bitmask to zero upon failure, there is no need for the powerMgmt_valid field.
* src/util/virnodesuspend.c: Ensure *bitmask is zero upon failure * src/conf/capabilities.c, src/conf/capabilities.h: Remove powerMgmt_valid field * src/qemu/qemu_capabilities.c: Remove powerMgmt_valid --- src/conf/capabilities.c | 30 ++++++++++++++---------------- src/conf/capabilities.h | 1 - src/qemu/qemu_capabilities.c | 5 +---- src/util/virnodesuspend.c | 10 +++++++--- 4 files changed, 22 insertions(+), 24 deletions(-)
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index df5ff23..ac050df 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -696,23 +696,21 @@ virCapabilitiesFormatXML(virCapsPtr caps)
virBufferAddLit(&xml, " </cpu>\n");
- if (caps->host.powerMgmt_valid) { - /* The PM query was successful. */ - if (caps->host.powerMgmt) { - /* The host supports some PM features. */ - unsigned int pm = caps->host.powerMgmt; - virBufferAddLit(&xml, " <power_management>\n"); - while (pm) { - int bit = ffs(pm) - 1; - virBufferAsprintf(&xml, " <%s/>\n", - virCapsHostPMTargetTypeToString(bit)); - pm &= ~(1U << bit); - } - virBufferAddLit(&xml, " </power_management>\n"); - } else { - /* The host does not support any PM feature. */ - virBufferAddLit(&xml, " <power_management/>\n"); + /* The PM query was successful. */
^^^^^^^^^^^^^ Considering the point that Eric brought out about the original design of the <power_management> tag (which I also intended to bring out in fact), if you decide to go with the new design, the comment above might not make much sense. We might as well remove it.
+ if (caps->host.powerMgmt) { + /* The host supports some PM features. */ + unsigned int pm = caps->host.powerMgmt; + virBufferAddLit(&xml, " <power_management>\n"); + while (pm) { + int bit = ffs(pm) - 1; + virBufferAsprintf(&xml, " <%s/>\n", + virCapsHostPMTargetTypeToString(bit)); + pm &= ~(1U << bit); } + virBufferAddLit(&xml, " </power_management>\n"); + } else { + /* The host does not support any PM feature. */ + virBufferAddLit(&xml, " <power_management/>\n"); }
-- Regards, Srivatsa S. Bhat IBM Linux Technology Center
From: "Daniel P. Berrange" <berrange@redhat.com> To avoid probing the host power management features on any call to virInitialize, only initialize the mutex in virNodeSuspendInit. Do lazy load of the supported PM target mask when it is actually needed * src/util/virnodesuspend.c: Lazy init of supported features --- src/util/virnodesuspend.c | 99 +++++++++++++++++++++++---------------------- 1 files changed, 50 insertions(+), 49 deletions(-) diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c index 6420f5b..75a8c2f 100644 --- a/src/util/virnodesuspend.c +++ b/src/util/virnodesuspend.c @@ -46,9 +46,10 @@ * Bitmask to hold the Power Management features supported by the host, * such as Suspend-to-RAM, Suspend-to-Disk, Hybrid-Suspend etc. */ -static unsigned int hostPMFeatures; +static unsigned int nodeSuspendTargetMask = 0; +static bool nodeSuspendTargetMaskInit = false; -virMutex virNodeSuspendMutex; +static virMutex virNodeSuspendMutex; static bool aboutToSuspend; @@ -75,17 +76,9 @@ static void virNodeSuspendUnlock(void) */ int virNodeSuspendInit(void) { - if (virMutexInit(&virNodeSuspendMutex) < 0) return -1; - /* Get the power management capabilities supported by the host */ - hostPMFeatures = 0; - if (virNodeSuspendGetTargetMask(&hostPMFeatures) < 0) { - if (geteuid() == 0) - VIR_ERROR(_("Failed to get host power management features")); - } - return 0; } @@ -191,9 +184,14 @@ int nodeSuspendForDuration(virConnectPtr conn ATTRIBUTE_UNUSED, { static virThread thread; char *cmdString = NULL; + int ret = -1; + unsigned int supported; virCheckFlags(0, -1); + if (virNodeSuspendGetTargetMask(&supported) < 0) + return -1; + /* * Ensure that we are the only ones trying to suspend. * Fail if somebody has already initiated a suspend. @@ -202,18 +200,16 @@ int nodeSuspendForDuration(virConnectPtr conn ATTRIBUTE_UNUSED, if (aboutToSuspend) { /* A suspend operation is already in progress */ - virNodeSuspendUnlock(); - return -1; - } else { - aboutToSuspend = true; + virNodeSuspendError(VIR_ERR_OPERATION_INVALID, "%s", + _("Suspend operation already in progress")); + goto cleanup; } - - virNodeSuspendUnlock(); + aboutToSuspend = true; /* Check if the host supports the requested suspend target */ switch (target) { case VIR_NODE_SUSPEND_TARGET_MEM: - if (hostPMFeatures & (1 << VIR_NODE_SUSPEND_TARGET_MEM)) { + if (supported & (1 << VIR_NODE_SUSPEND_TARGET_MEM)) { cmdString = strdup("pm-suspend"); if (cmdString == NULL) { virReportOOMError(); @@ -225,7 +221,7 @@ int nodeSuspendForDuration(virConnectPtr conn ATTRIBUTE_UNUSED, goto cleanup; case VIR_NODE_SUSPEND_TARGET_DISK: - if (hostPMFeatures & (1 << VIR_NODE_SUSPEND_TARGET_DISK)) { + if (supported & (1 << VIR_NODE_SUSPEND_TARGET_DISK)) { cmdString = strdup("pm-hibernate"); if (cmdString == NULL) { virReportOOMError(); @@ -237,7 +233,7 @@ int nodeSuspendForDuration(virConnectPtr conn ATTRIBUTE_UNUSED, goto cleanup; case VIR_NODE_SUSPEND_TARGET_HYBRID: - if (hostPMFeatures & (1 << VIR_NODE_SUSPEND_TARGET_HYBRID)) { + if (supported & (1 << VIR_NODE_SUSPEND_TARGET_HYBRID)) { cmdString = strdup("pm-suspend-hybrid"); if (cmdString == NULL) { virReportOOMError(); @@ -263,11 +259,11 @@ int nodeSuspendForDuration(virConnectPtr conn ATTRIBUTE_UNUSED, goto cleanup; } - return 0; - + ret = 0; cleanup: + virNodeSuspendUnlock(); VIR_FREE(cmdString); - return -1; + return ret; } @@ -338,35 +334,40 @@ cleanup: int virNodeSuspendGetTargetMask(unsigned int *bitmask) { - int ret; - bool supported; + int ret = -1; *bitmask = 0; - /* Check support for Suspend-to-RAM (S3) */ - ret = virNodeSuspendSupportsTarget(VIR_NODE_SUSPEND_TARGET_MEM, &supported); - if (ret < 0) - goto error; - if (supported) - *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_MEM); - - /* Check support for Suspend-to-Disk (S4) */ - ret = virNodeSuspendSupportsTarget(VIR_NODE_SUSPEND_TARGET_DISK, &supported); - if (ret < 0) - goto error; - if (supported) - *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_DISK); - - /* Check support for Hybrid-Suspend */ - ret = virNodeSuspendSupportsTarget(VIR_NODE_SUSPEND_TARGET_HYBRID, &supported); - if (ret < 0) - goto error; - if (supported) - *bitmask |= (1 << VIR_NODE_SUSPEND_TARGET_HYBRID); - - return 0; + virNodeSuspendLock(); + /* Get the power management capabilities supported by the host */ + if (!nodeSuspendTargetMaskInit) { + bool supported; + nodeSuspendTargetMask = 0; + + /* Check support for Suspend-to-RAM (S3) */ + if (virNodeSuspendSupportsTarget(VIR_NODE_SUSPEND_TARGET_MEM, &supported) < 0) + goto cleanup; + if (supported) + nodeSuspendTargetMask |= (1 << VIR_NODE_SUSPEND_TARGET_MEM); + + /* Check support for Suspend-to-Disk (S4) */ + if (virNodeSuspendSupportsTarget(VIR_NODE_SUSPEND_TARGET_DISK, &supported) < 0) + goto cleanup; + if (supported) + nodeSuspendTargetMask |= (1 << VIR_NODE_SUSPEND_TARGET_DISK); + + /* Check support for Hybrid-Suspend */ + if (virNodeSuspendSupportsTarget(VIR_NODE_SUSPEND_TARGET_HYBRID, &supported) < 0) + goto cleanup; + if (supported) + nodeSuspendTargetMask |= (1 << VIR_NODE_SUSPEND_TARGET_HYBRID); + + nodeSuspendTargetMaskInit = true; + } -error: - *bitmask = 0; - return -1; + *bitmask = nodeSuspendTargetMask; + ret = 0; +cleanup: + virNodeSuspendUnlock(); + return ret; } -- 1.7.6.4
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
To avoid probing the host power management features on any call to virInitialize, only initialize the mutex in virNodeSuspendInit. Do lazy load of the supported PM target mask when it is actually needed
* src/util/virnodesuspend.c: Lazy init of supported features --- src/util/virnodesuspend.c | 99 +++++++++++++++++++++++---------------------- 1 files changed, 50 insertions(+), 49 deletions(-)
diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c index 6420f5b..75a8c2f 100644 --- a/src/util/virnodesuspend.c +++ b/src/util/virnodesuspend.c @@ -46,9 +46,10 @@ * Bitmask to hold the Power Management features supported by the host, * such as Suspend-to-RAM, Suspend-to-Disk, Hybrid-Suspend etc. */ -static unsigned int hostPMFeatures; +static unsigned int nodeSuspendTargetMask = 0; +static bool nodeSuspendTargetMaskInit = false;
C89 and later guarantee that static variables are 0-initialized unless you state otherwise. Meanwhile, explicit initialization penalizes old compilers that stick the variables into .data instead of .bss. gcc is smart enough to generate 0-init'd variables into .bss for smaller executables, but since not all compilers are that smart, you may want to remove these explicit initializations to 0. ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> The command name for the suspend action does not need to be strdup'd. The constant string can be used directly. This also means the code can be trivially rearranged to make the switch clearer * src/util/virnodesuspend.c: Remove strdup of cmdString --- src/util/virnodesuspend.c | 49 +++++++++++++++----------------------------- 1 files changed, 17 insertions(+), 32 deletions(-) diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c index 75a8c2f..3805e9c 100644 --- a/src/util/virnodesuspend.c +++ b/src/util/virnodesuspend.c @@ -127,9 +127,7 @@ cleanup: */ static void virNodeSuspend(void *cmdString) { - virCommandPtr suspendCmd = virCommandNew((char *)cmdString); - - VIR_FREE(cmdString); + virCommandPtr suspendCmd = virCommandNew((const char *)cmdString); /* * Delay for sometime so that the function nodeSuspendForDuration() @@ -183,7 +181,7 @@ int nodeSuspendForDuration(virConnectPtr conn ATTRIBUTE_UNUSED, unsigned int flags) { static virThread thread; - char *cmdString = NULL; + const char *cmdString = NULL; int ret = -1; unsigned int supported; @@ -209,40 +207,28 @@ int nodeSuspendForDuration(virConnectPtr conn ATTRIBUTE_UNUSED, /* Check if the host supports the requested suspend target */ switch (target) { case VIR_NODE_SUSPEND_TARGET_MEM: - if (supported & (1 << VIR_NODE_SUSPEND_TARGET_MEM)) { - cmdString = strdup("pm-suspend"); - if (cmdString == NULL) { - virReportOOMError(); - goto cleanup; - } - break; + if (!(supported & (1 << VIR_NODE_SUSPEND_TARGET_MEM))) { + virNodeSuspendError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Suspend-to-RAM")); + goto cleanup; } - virNodeSuspendError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Suspend-to-RAM")); - goto cleanup; + cmdString = "pm-suspend"; + break; case VIR_NODE_SUSPEND_TARGET_DISK: - if (supported & (1 << VIR_NODE_SUSPEND_TARGET_DISK)) { - cmdString = strdup("pm-hibernate"); - if (cmdString == NULL) { - virReportOOMError(); - goto cleanup; - } - break; + if (!(supported & (1 << VIR_NODE_SUSPEND_TARGET_DISK))) { + virNodeSuspendError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Suspend-to-Disk")); + goto cleanup; } - virNodeSuspendError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Suspend-to-Disk")); - goto cleanup; + cmdString = "pm-hibernate"; + break; case VIR_NODE_SUSPEND_TARGET_HYBRID: - if (supported & (1 << VIR_NODE_SUSPEND_TARGET_HYBRID)) { - cmdString = strdup("pm-suspend-hybrid"); - if (cmdString == NULL) { - virReportOOMError(); - goto cleanup; - } - break; + if (!(supported & (1 << VIR_NODE_SUSPEND_TARGET_HYBRID))) { + virNodeSuspendError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Hybrid-Suspend")); + goto cleanup; } - virNodeSuspendError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Hybrid-Suspend")); - goto cleanup; + cmdString = "pm-suspend-hybrid"; + break; default: virNodeSuspendError(VIR_ERR_INVALID_ARG, "%s", _("Invalid suspend target")); @@ -262,7 +248,6 @@ int nodeSuspendForDuration(virConnectPtr conn ATTRIBUTE_UNUSED, ret = 0; cleanup: virNodeSuspendUnlock(); - VIR_FREE(cmdString); return ret; } -- 1.7.6.4
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The command name for the suspend action does not need to be strdup'd. The constant string can be used directly. This also means the code can be trivially rearranged to make the switch clearer
And one less point of OOM failure.
* src/util/virnodesuspend.c: Remove strdup of cmdString --- src/util/virnodesuspend.c | 49 +++++++++++++++----------------------------- 1 files changed, 17 insertions(+), 32 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> * src/lxc/lxc_conf.c, src/uml/uml_conf.c, src/xen/xen_hypervisor.c: Initialize suspend capabilities * tests/xencapsdata/*xml: Add empty powermgmt capabilities --- src/lxc/lxc_conf.c | 5 +++++ src/uml/uml_conf.c | 5 +++++ src/xen/xen_hypervisor.c | 4 ++++ tests/xencapsdata/xen-i686-pae-hvm.xml | 1 + tests/xencapsdata/xen-i686-pae.xml | 1 + tests/xencapsdata/xen-i686.xml | 1 + tests/xencapsdata/xen-ia64-be-hvm.xml | 1 + tests/xencapsdata/xen-ia64-be.xml | 1 + tests/xencapsdata/xen-ia64-hvm.xml | 1 + tests/xencapsdata/xen-ia64.xml | 1 + tests/xencapsdata/xen-ppc64.xml | 1 + tests/xencapsdata/xen-x86_64-hvm.xml | 1 + tests/xencapsdata/xen-x86_64.xml | 1 + 13 files changed, 24 insertions(+), 0 deletions(-) diff --git a/src/lxc/lxc_conf.c b/src/lxc/lxc_conf.c index 52e99ca..e842736 100644 --- a/src/lxc/lxc_conf.c +++ b/src/lxc/lxc_conf.c @@ -37,6 +37,8 @@ #include "uuid.h" #include "configmake.h" #include "lxc_container.h" +#include "virnodesuspend.h" + #define VIR_FROM_THIS VIR_FROM_LXC @@ -71,6 +73,9 @@ virCapsPtr lxcCapsInit(void) VIR_WARN("Failed to query host NUMA topology, disabling NUMA capabilities"); } + if (virNodeSuspendGetTargetMask(&caps->host.powerMgmt) < 0) + VIR_WARN("Failed to get host power management capabilities"); + if (virGetHostUUID(caps->host.host_uuid)) { lxcError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot get the host uuid")); diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index d4cb12b..a576047 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -47,6 +47,8 @@ #include "virfile.h" #include "command.h" #include "virnetdevtap.h" +#include "virnodesuspend.h" + #define VIR_FROM_THIS VIR_FROM_UML @@ -81,6 +83,9 @@ virCapsPtr umlCapsInit(void) { VIR_WARN("Failed to query host NUMA topology, disabling NUMA capabilities"); } + if (virNodeSuspendGetTargetMask(&caps->host.powerMgmt) < 0) + VIR_WARN("Failed to get host power management capabilities"); + if (virGetHostUUID(caps->host.host_uuid)) { umlReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot get the host uuid")); diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index 870bc4f..6c71b65 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -66,6 +66,7 @@ #include "capabilities.h" #include "memory.h" #include "virfile.h" +#include "virnodesuspend.h" #define VIR_FROM_THIS VIR_FROM_XEN @@ -2743,6 +2744,9 @@ xenHypervisorMakeCapabilities(virConnectPtr conn) cpuinfo, capabilities); + if (virNodeSuspendGetTargetMask(&caps->host.powerMgmt) < 0) + VIR_WARN("Failed to get host power management capabilities"); + VIR_FORCE_FCLOSE(cpuinfo); VIR_FORCE_FCLOSE(capabilities); diff --git a/tests/xencapsdata/xen-i686-pae-hvm.xml b/tests/xencapsdata/xen-i686-pae-hvm.xml index 42b099c..872e5f6 100644 --- a/tests/xencapsdata/xen-i686-pae-hvm.xml +++ b/tests/xencapsdata/xen-i686-pae-hvm.xml @@ -7,6 +7,7 @@ <vmx/> </features> </cpu> + <power_management/> <migration_features> <live/> <uri_transports> diff --git a/tests/xencapsdata/xen-i686-pae.xml b/tests/xencapsdata/xen-i686-pae.xml index a6cec8a..3dba6eb 100644 --- a/tests/xencapsdata/xen-i686-pae.xml +++ b/tests/xencapsdata/xen-i686-pae.xml @@ -7,6 +7,7 @@ <vmx/> </features> </cpu> + <power_management/> <migration_features> <live/> <uri_transports> diff --git a/tests/xencapsdata/xen-i686.xml b/tests/xencapsdata/xen-i686.xml index 9071212..22d7685 100644 --- a/tests/xencapsdata/xen-i686.xml +++ b/tests/xencapsdata/xen-i686.xml @@ -4,6 +4,7 @@ <cpu> <arch>i686</arch> </cpu> + <power_management/> <migration_features> <live/> <uri_transports> diff --git a/tests/xencapsdata/xen-ia64-be-hvm.xml b/tests/xencapsdata/xen-ia64-be-hvm.xml index 732b693..222de1d 100644 --- a/tests/xencapsdata/xen-ia64-be-hvm.xml +++ b/tests/xencapsdata/xen-ia64-be-hvm.xml @@ -4,6 +4,7 @@ <cpu> <arch>ia64</arch> </cpu> + <power_management/> <migration_features> <live/> <uri_transports> diff --git a/tests/xencapsdata/xen-ia64-be.xml b/tests/xencapsdata/xen-ia64-be.xml index 4f133ec..017816c 100644 --- a/tests/xencapsdata/xen-ia64-be.xml +++ b/tests/xencapsdata/xen-ia64-be.xml @@ -4,6 +4,7 @@ <cpu> <arch>ia64</arch> </cpu> + <power_management/> <migration_features> <live/> <uri_transports> diff --git a/tests/xencapsdata/xen-ia64-hvm.xml b/tests/xencapsdata/xen-ia64-hvm.xml index ef48a95..33c4946 100644 --- a/tests/xencapsdata/xen-ia64-hvm.xml +++ b/tests/xencapsdata/xen-ia64-hvm.xml @@ -4,6 +4,7 @@ <cpu> <arch>ia64</arch> </cpu> + <power_management/> <migration_features> <live/> <uri_transports> diff --git a/tests/xencapsdata/xen-ia64.xml b/tests/xencapsdata/xen-ia64.xml index 5570f4d..82ce965 100644 --- a/tests/xencapsdata/xen-ia64.xml +++ b/tests/xencapsdata/xen-ia64.xml @@ -4,6 +4,7 @@ <cpu> <arch>ia64</arch> </cpu> + <power_management/> <migration_features> <live/> <uri_transports> diff --git a/tests/xencapsdata/xen-ppc64.xml b/tests/xencapsdata/xen-ppc64.xml index 627d79c..91401b9 100644 --- a/tests/xencapsdata/xen-ppc64.xml +++ b/tests/xencapsdata/xen-ppc64.xml @@ -4,6 +4,7 @@ <cpu> <arch>ppc64</arch> </cpu> + <power_management/> <migration_features> <live/> <uri_transports> diff --git a/tests/xencapsdata/xen-x86_64-hvm.xml b/tests/xencapsdata/xen-x86_64-hvm.xml index 52c12c6..8de8cf4 100644 --- a/tests/xencapsdata/xen-x86_64-hvm.xml +++ b/tests/xencapsdata/xen-x86_64-hvm.xml @@ -7,6 +7,7 @@ <svm/> </features> </cpu> + <power_management/> <migration_features> <live/> <uri_transports> diff --git a/tests/xencapsdata/xen-x86_64.xml b/tests/xencapsdata/xen-x86_64.xml index 0faa43c..0c3279b 100644 --- a/tests/xencapsdata/xen-x86_64.xml +++ b/tests/xencapsdata/xen-x86_64.xml @@ -7,6 +7,7 @@ <svm/> </features> </cpu> + <power_management/> <migration_features> <live/> <uri_transports> -- 1.7.6.4
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
* src/lxc/lxc_conf.c, src/uml/uml_conf.c, src/xen/xen_hypervisor.c: Initialize suspend capabilities * tests/xencapsdata/*xml: Add empty powermgmt capabilities
Do we need to fake anything in the tests directory to ensure that the tests never probe the system, but always treat things as no power management available? I'm worried that running the testsuite as root on a Dom0 host might cause spurious test failures, although I have not yet tested that. I guess we can deal with the fallout later if it turns out to be a problem in practice.
@@ -71,6 +73,9 @@ virCapsPtr lxcCapsInit(void) VIR_WARN("Failed to query host NUMA topology, disabling NUMA capabilities"); }
+ if (virNodeSuspendGetTargetMask(&caps->host.powerMgmt) < 0) + VIR_WARN("Failed to get host power management capabilities"); +
For qemu, you skipped the warning for qemu:///session (ie. geteuid() was non-zero). Should we do the same for lxc and uml, or are these hypervisors only ever useful as root? -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Nov 29, 2011 at 09:56:43AM -0700, Eric Blake wrote:
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
* src/lxc/lxc_conf.c, src/uml/uml_conf.c, src/xen/xen_hypervisor.c: Initialize suspend capabilities * tests/xencapsdata/*xml: Add empty powermgmt capabilities
Do we need to fake anything in the tests directory to ensure that the tests never probe the system, but always treat things as no power management available? I'm worried that running the testsuite as root on a Dom0 host might cause spurious test failures, although I have not yet tested that. I guess we can deal with the fallout later if it turns out to be a problem in practice.
Yes, I already made that mistake, and this patch ensures it does not get run during test suites :-)
@@ -71,6 +73,9 @@ virCapsPtr lxcCapsInit(void) VIR_WARN("Failed to query host NUMA topology, disabling NUMA capabilities"); }
+ if (virNodeSuspendGetTargetMask(&caps->host.powerMgmt) < 0) + VIR_WARN("Failed to get host power management capabilities"); +
For qemu, you skipped the warning for qemu:///session (ie. geteuid() was non-zero). Should we do the same for lxc and uml, or are these hypervisors only ever useful as root?
Actually it doesn't fail for non-root - the commands for querying supported PM features are fine to run as non-root. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 11/29/2011 10:50 AM, Daniel P. Berrange wrote:
On Tue, Nov 29, 2011 at 09:56:43AM -0700, Eric Blake wrote:
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
* src/lxc/lxc_conf.c, src/uml/uml_conf.c, src/xen/xen_hypervisor.c: Initialize suspend capabilities * tests/xencapsdata/*xml: Add empty powermgmt capabilities
Do we need to fake anything in the tests directory to ensure that the tests never probe the system, but always treat things as no power management available? I'm worried that running the testsuite as root on a Dom0 host might cause spurious test failures, although I have not yet tested that. I guess we can deal with the fallout later if it turns out to be a problem in practice.
Yes, I already made that mistake, and this patch ensures it does not get run during test suites :-)
@@ -71,6 +73,9 @@ virCapsPtr lxcCapsInit(void) VIR_WARN("Failed to query host NUMA topology, disabling NUMA capabilities"); }
+ if (virNodeSuspendGetTargetMask(&caps->host.powerMgmt) < 0) + VIR_WARN("Failed to get host power management capabilities"); +
For qemu, you skipped the warning for qemu:///session (ie. geteuid() was non-zero). Should we do the same for lxc and uml, or are these hypervisors only ever useful as root?
Actually it doesn't fail for non-root - the commands for querying supported PM features are fine to run as non-root.
All right, then you've answered my questions, so for the record: ACK. I think that completes the series review, with only a few nits to fix before you push. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> If suspend failed for some reason (eg too short duration) then susquent attempts to trigger suspend were rejected because we had already marked a suspend as being in progress * src/util/virnodesuspend.c: Don't mark suspend as active until we've successfully triggered it --- src/util/virnodesuspend.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c index 3805e9c..f585847 100644 --- a/src/util/virnodesuspend.c +++ b/src/util/virnodesuspend.c @@ -202,7 +202,6 @@ int nodeSuspendForDuration(virConnectPtr conn ATTRIBUTE_UNUSED, _("Suspend operation already in progress")); goto cleanup; } - aboutToSuspend = true; /* Check if the host supports the requested suspend target */ switch (target) { @@ -245,6 +244,7 @@ int nodeSuspendForDuration(virConnectPtr conn ATTRIBUTE_UNUSED, goto cleanup; } + aboutToSuspend = true; ret = 0; cleanup: virNodeSuspendUnlock(); -- 1.7.6.4
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
If suspend failed for some reason (eg too short duration) then
s/eg/e.g./
susquent attempts to trigger suspend were rejected because we
s/susquent/subsequent/
had already marked a suspend as being in progress
* src/util/virnodesuspend.c: Don't mark suspend as active until we've successfully triggered it --- src/util/virnodesuspend.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
ACK. As long as we only mark aboutToSuspend while holding the lock, and only ever probe it while also holding the lock, then we're safe. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Nov 29, 2011 at 03:44:35PM +0000, Daniel P. Berrange wrote:
This patch series does a large number of pretty small / trivial cleanups of the recently committed node suspend code.
There are a few important changes though
- Patch 1 changes public API enum values - Patch 3 changes the capabilities XML schema - Patch 11 ensures we don't probe suspend info unles we need it - Patch 14 fixes a bug
Obviously due to 1 & 3 we need to do this before the next release
ACK to the serie ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Srivatsa S. Bhat