10:44 a.m.
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
10:44 a.m.
New subject: [libvirt] [PATCH 01/14] Fix values of PM target type constants
From: "Daniel P. Berrange" <berrange(a)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
11:11 a.m.
New subject: [libvirt] [PATCH 01/14] Fix values of PM target type constants
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:44 a.m.
New subject: [libvirt] [PATCH 02/14] Remove internal only virHostPMCapability enum
From: "Daniel P. Berrange" <berrange(a)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
11:13 a.m.
New subject: [libvirt] [PATCH 02/14] Remove internal only virHostPMCapability enum
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:14 a.m.
New subject: [libvirt] [PATCH 02/14] Remove internal only virHostPMCapability enum
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(a)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 :|
10:44 a.m.
New subject: [libvirt] [PATCH 03/14] Fix capabilities XML to use generic terms for suspend targets
From: "Daniel P. Berrange" <berrange(a)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
11:16 a.m.
New subject: [libvirt] [PATCH 03/14] Fix capabilities XML to use generic terms for suspend targets
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:44 a.m.
New subject: [libvirt] [PATCH 04/14] Move the virHostPMCapability enum helpers into capabilities.c
From: "Daniel P. Berrange" <berrange(a)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
11:17 a.m.
New subject: [libvirt] [PATCH 04/14] Move the virHostPMCapability enum helpers into capabilities.c
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:44 a.m.
New subject: [libvirt] [PATCH 05/14] Sanitize virDiscoverHostPMFeature to return a boolean
From: "Daniel P. Berrange" <berrange(a)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
11:20 a.m.
New subject: [libvirt] [PATCH 05/14] Sanitize virDiscoverHostPMFeature to return a boolean
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:11 p.m.
New subject: [libvirt] [PATCH 05/14] Sanitize virDiscoverHostPMFeature to return a boolean
On 11/29/2011 09:14 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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
10:44 a.m.
New subject: [libvirt] [PATCH 06/14] Rename suspend capabilities APIs
From: "Daniel P. Berrange" <berrange(a)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
11:19 a.m.
New subject: [libvirt] [PATCH 06/14] Rename suspend capabilities APIs
On 11/29/2011 09:14 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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
11:23 a.m.
New subject: [libvirt] [PATCH 06/14] Rename suspend capabilities APIs
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:28 a.m.
New subject: [libvirt] [PATCH 06/14] Rename suspend capabilities APIs
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
11:22 a.m.
New subject: [libvirt] [PATCH 06/14] Rename suspend capabilities APIs
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)redhat.com>
Rename virGetPMCapabilities to virNodeSuspendGetTargetMask and
virDiscoverHostPMFeature to virNodeSuspendSupportsTarget.
Makes sense for more consistent naming.
ACK for mechanical rename.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:44 a.m.
New subject: [libvirt] [PATCH 07/14] Move suspend capabilities APIs out of util.h into virnodesuspend.c
From: "Daniel P. Berrange" <berrange(a)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
11:30 a.m.
New subject: [libvirt] [PATCH 07/14] Move suspend capabilities APIs out of util.h into virnodesuspend.c
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:01 p.m.
New subject: [libvirt] [PATCH 07/14] Move suspend capabilities APIs out of util.h into virnodesuspend.c
On 11/29/2011 09:14 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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
5:01 a.m.
New subject: [libvirt] [PATCH 07/14] Move suspend capabilities APIs out of util.h into virnodesuspend.c
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(a)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 :|
10:44 a.m.
New subject: [libvirt] [PATCH 08/14] Add export of node suspend capabilities APIs
From: "Daniel P. Berrange" <berrange(a)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
11:31 a.m.
New subject: [libvirt] [PATCH 08/14] Add export of node suspend capabilities APIs
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:03 p.m.
New subject: [libvirt] [PATCH 08/14] Add export of node suspend capabilities APIs
On 11/29/2011 09:14 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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
10:44 a.m.
New subject: [libvirt] [PATCH 09/14] Fix check for whether a suspend target is supported
From: "Daniel P. Berrange" <berrange(a)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
11:33 a.m.
New subject: [libvirt] [PATCH 09/14] Fix check for whether a suspend target is supported
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:40 a.m.
New subject: [libvirt] [PATCH 09/14] Fix check for whether a suspend target is supported
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(a)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 :|
11:53 a.m.
New subject: [libvirt] [PATCH 09/14] Fix check for whether a suspend target is supported
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(a)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
12:05 p.m.
New subject: [libvirt] [PATCH 09/14] Fix check for whether a suspend target is supported
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(a)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 :|
10:44 a.m.
New subject: [libvirt] [PATCH 10/14] Remove powerMgmt_valid field from capabilities struct
From: "Daniel P. Berrange" <berrange(a)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
11:40 a.m.
New subject: [libvirt] [PATCH 10/14] Remove powerMgmt_valid field from capabilities struct
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:23 p.m.
New subject: [libvirt] [PATCH 10/14] Remove powerMgmt_valid field from capabilities struct
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(a)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 :|
12:03 p.m.
New subject: [libvirt] [PATCH 10/14] Remove powerMgmt_valid field from capabilities struct
On 11/29/2011 09:14 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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
10:44 a.m.
New subject: [libvirt] [PATCH 11/14] Do lazy init of host PM features
From: "Daniel P. Berrange" <berrange(a)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
11:48 a.m.
New subject: [libvirt] [PATCH 11/14] Do lazy init of host PM features
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:44 a.m.
New subject: [libvirt] [PATCH 12/14] Remove pointless strdup in node suspend code
From: "Daniel P. Berrange" <berrange(a)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
11:52 a.m.
New subject: [libvirt] [PATCH 12/14] Remove pointless strdup in node suspend code
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:44 a.m.
New subject: [libvirt] [PATCH 13/14] Add suspend info to Xen, LXC and UML hypervisor capabilities
From: "Daniel P. Berrange" <berrange(a)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
11:56 a.m.
New subject: [libvirt] [PATCH 13/14] Add suspend info to Xen, LXC and UML hypervisor capabilities
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:50 p.m.
New subject: [libvirt] [PATCH 13/14] Add suspend info to Xen, LXC and UML hypervisor capabilities
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(a)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 :|
12:59 p.m.
New subject: [libvirt] [PATCH 13/14] Add suspend info to Xen, LXC and UML hypervisor capabilities
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(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:44 a.m.
New subject: [libvirt] [PATCH 14/14] Don't mark suspend as active until we know it is running
From: "Daniel P. Berrange" <berrange(a)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
12:09 p.m.
New subject: [libvirt] [PATCH 14/14] Don't mark suspend as active until we know it is running
On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
2:46 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
4935
days inactive
4936
days old
44 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Srivatsa S. Bhat