11:32 a.m.
From: Cédric Bosdonnat <cedric.bosdonnat(a)free.fr>
pm-is-supported is the only thing needed in pm-utils, better get rid of
it since systemd is heavily used for libvirt.
---
src/util/virnodesuspend.c | 34 ++++++++++++++++++++--------------
1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c
index 8088931..9839de2 100644
--- a/src/util/virnodesuspend.c
+++ b/src/util/virnodesuspend.c
@@ -29,6 +29,8 @@
#include "viralloc.h"
#include "virlog.h"
#include "virerror.h"
+#include "virfile.h"
+#include "virstring.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -263,41 +265,45 @@ int nodeSuspendForDuration(unsigned int target,
static int
virNodeSuspendSupportsTarget(unsigned int target, bool *supported)
{
- virCommandPtr cmd;
- int status;
int ret = -1;
+ char *buf = NULL;
+ char **states = NULL;
+ bool canSuspend = false;
+ bool canHibernate = false;
if (virNodeSuspendInitialize() < 0)
return -1;
*supported = false;
+ if (virFileReadAll("/sys/power/state", 1024, &buf) < 0)
+ goto cleanup;
+
+ states = virStringSplit(buf, " ", 0);
+
+ canSuspend = (virStringArrayHasString(states, "mem") ||
+ virStringArrayHasString(states, "standby"));
+ canHibernate = virStringArrayHasString(states, "disk");
+
switch (target) {
case VIR_NODE_SUSPEND_TARGET_MEM:
- cmd = virCommandNewArgList("pm-is-supported", "--suspend",
NULL);
+ *supported = canSuspend;
break;
case VIR_NODE_SUSPEND_TARGET_DISK:
- cmd = virCommandNewArgList("pm-is-supported", "--hibernate",
NULL);
+ *supported = canHibernate;
break;
case VIR_NODE_SUSPEND_TARGET_HYBRID:
- cmd = virCommandNewArgList("pm-is-supported",
"--suspend-hybrid", NULL);
+ *supported = canSuspend && canHibernate;
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);
+ VIR_FREE(buf);
+ virStringFreeList(states);
return ret;
}
--
1.8.4.5
2:49 p.m.
On Mar 28, 2014, at 11:32 AM, Cédric Bosdonnat <cbosdonnat(a)suse.com> wrote:
From: Cédric Bosdonnat <cedric.bosdonnat(a)free.fr>
pm-is-supported is the only thing needed in pm-utils, better get rid of
it since systemd is heavily used for libvirt.
---
src/util/virnodesuspend.c | 34 ++++++++++++++++++++--------------
1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c
index 8088931..9839de2 100644
--- a/src/util/virnodesuspend.c
+++ b/src/util/virnodesuspend.c
@@ -29,6 +29,8 @@
#include "viralloc.h"
#include "virlog.h"
#include "virerror.h"
+#include "virfile.h"
+#include "virstring.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -263,41 +265,45 @@ int nodeSuspendForDuration(unsigned int target,
static int
virNodeSuspendSupportsTarget(unsigned int target, bool *supported)
{
- virCommandPtr cmd;
- int status;
int ret = -1;
+ char *buf = NULL;
+ char **states = NULL;
+ bool canSuspend = false;
+ bool canHibernate = false;
if (virNodeSuspendInitialize() < 0)
return -1;
*supported = false;
+ if (virFileReadAll("/sys/power/state", 1024, &buf) < 0)
+ goto cleanup;
+
+ states = virStringSplit(buf, " ", 0);
+
+ canSuspend = (virStringArrayHasString(states, "mem") ||
+ virStringArrayHasString(states, "standby"));
+ canHibernate = virStringArrayHasString(states, "disk");
+
switch (target) {
case VIR_NODE_SUSPEND_TARGET_MEM:
- cmd = virCommandNewArgList("pm-is-supported", "--suspend",
NULL);
+ *supported = canSuspend;
break;
case VIR_NODE_SUSPEND_TARGET_DISK:
- cmd = virCommandNewArgList("pm-is-supported", "--hibernate",
NULL);
+ *supported = canHibernate;
break;
case VIR_NODE_SUSPEND_TARGET_HYBRID:
- cmd = virCommandNewArgList("pm-is-supported",
"--suspend-hybrid", NULL);
+ *supported = canSuspend && canHibernate;
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);
+ VIR_FREE(buf);
+ virStringFreeList(states);
return ret;
}
--
1.8.4.5
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
While systemd is used for a number of things there are other platforms than Linux and
there are Linux platforms that don't use systemd. Can't we just wrap this in if
not systemd?
--
Doug
3:01 p.m.
On 03/28/2014 01:49 PM, Doug Goldstein wrote:
On Mar 28, 2014, at 11:32 AM, Cédric Bosdonnat <cbosdonnat(a)suse.com> wrote:
>
> From: Cédric Bosdonnat <cedric.bosdonnat(a)free.fr>
>
> pm-is-supported is the only thing needed in pm-utils, better get rid of
> it since systemd is heavily used for libvirt.
> ---
> src/util/virnodesuspend.c | 34 ++++++++++++++++++++--------------
> 1 file changed, 20 insertions(+), 14 deletions(-)
>
>
> + if (virFileReadAll("/sys/power/state", 1024, &buf) < 0)
> + goto cleanup;
> +
> case VIR_NODE_SUSPEND_TARGET_MEM:
> - cmd = virCommandNewArgList("pm-is-supported",
"--suspend", NULL);
> + *supported = canSuspend;
While systemd is used for a number of things there are other
platforms than Linux and there are Linux platforms that don't use systemd. Can't
we just wrap this in if not systemd?
How long has the kernel been providing /sys/power/state and
/sys/power/disk? It looks like pm-is-supported is just a shell script
that greps these two files - so it makes total sense to me to likewise
just search these files ourselves instead of going through a shell
script to do it on our behalf. Thus, this patch is independent of
whether we use systemd, and more a case of avoiding a dependence on a
package for the use of a single shell script, especially for platforms
where systemd is taking over most other functionality in that package.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3:07 p.m.
On 03/28/2014 10:32 AM, Cédric Bosdonnat wrote:
From: Cédric Bosdonnat <cedric.bosdonnat(a)free.fr>
pm-is-supported is the only thing needed in pm-utils, better get rid of
it since systemd is heavily used for libvirt.
---
src/util/virnodesuspend.c | 34 ++++++++++++++++++++--------------
1 file changed, 20 insertions(+), 14 deletions(-)
You also need to modify libvirt.spec.in to drop the dependency.
+ if (virFileReadAll("/sys/power/state", 1024, &buf) < 0)
+ goto cleanup;
+
+ states = virStringSplit(buf, " ", 0);
+
+ canSuspend = (virStringArrayHasString(states, "mem") ||
+ virStringArrayHasString(states, "standby"));
+ canHibernate = virStringArrayHasString(states, "disk");
pm-is-supported checks a bit more than what your replacement checks.
For suspend, it declares yes if any of these succeed:
grep -q mem /sys/power/state
[ -c /dev/pmu ] && pm-pmu --check
grep -q standby /sys/power/state
For hibernate, it requires that BOTH of these succeed:
[ -f /sys/power/disk ]
grep -q disk /sys/power/state
For hybrid, it requires that all three succeed:
[ -f /sys/power/disk ] && \
grep -q disk /sys/power/state && \
grep -q suspend /sys/power/disk
as well as having fallback code to fake a hybrid sleep by joining the
other two states.
+
switch (target) {
case VIR_NODE_SUSPEND_TARGET_MEM:
- cmd = virCommandNewArgList("pm-is-supported", "--suspend",
NULL);
+ *supported = canSuspend;
break;
case VIR_NODE_SUSPEND_TARGET_DISK:
- cmd = virCommandNewArgList("pm-is-supported", "--hibernate",
NULL);
+ *supported = canHibernate;
break;
case VIR_NODE_SUSPEND_TARGET_HYBRID:
- cmd = virCommandNewArgList("pm-is-supported",
"--suspend-hybrid", NULL);
+ *supported = canSuspend && canHibernate;
I'm not sure if your simpler checks will cause us to declare an action
unsupported on systems where it was previously declared supported by
pm-is-supported. I think the idea makes sense, but I'd like a second
opinion that we aren't hurting ourselves by doing fewer checks than what
we are replacing.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:09 a.m.
Hi Eric,
On Fri, 2014-03-28 at 14:07 -0600, Eric Blake wrote:
On 03/28/2014 10:32 AM, Cédric Bosdonnat wrote:
> From: Cédric Bosdonnat <cedric.bosdonnat(a)free.fr>
>
> pm-is-supported is the only thing needed in pm-utils, better get rid of
> it since systemd is heavily used for libvirt.
> ---
> src/util/virnodesuspend.c | 34 ++++++++++++++++++++--------------
> 1 file changed, 20 insertions(+), 14 deletions(-)
You also need to modify libvirt.spec.in to drop the dependency.
Oh, yes, I forgot about this one.
>
>
> + if (virFileReadAll("/sys/power/state", 1024, &buf) < 0)
> + goto cleanup;
> +
> + states = virStringSplit(buf, " ", 0);
> +
> + canSuspend = (virStringArrayHasString(states, "mem") ||
> + virStringArrayHasString(states, "standby"));
> + canHibernate = virStringArrayHasString(states, "disk");
pm-is-supported checks a bit more than what your replacement checks.
For suspend, it declares yes if any of these succeed:
grep -q mem /sys/power/state
[ -c /dev/pmu ] && pm-pmu --check
grep -q standby /sys/power/state
The only missing bit here is the /dev/pmu one and that one is a powermac
only thing... so probably not something really bothering us.
For hibernate, it requires that BOTH of these succeed:
[ -f /sys/power/disk ]
grep -q disk /sys/power/state
Oh... I overlooked the /sys/power/disk check, I'll add it.
For hybrid, it requires that all three succeed:
[ -f /sys/power/disk ] && \
grep -q disk /sys/power/state && \
grep -q suspend /sys/power/disk
Same here.
as well as having fallback code to fake a hybrid sleep by joining
the
other two states.
> +
> switch (target) {
> case VIR_NODE_SUSPEND_TARGET_MEM:
> - cmd = virCommandNewArgList("pm-is-supported",
"--suspend", NULL);
> + *supported = canSuspend;
> break;
> case VIR_NODE_SUSPEND_TARGET_DISK:
> - cmd = virCommandNewArgList("pm-is-supported",
"--hibernate", NULL);
> + *supported = canHibernate;
> break;
> case VIR_NODE_SUSPEND_TARGET_HYBRID:
> - cmd = virCommandNewArgList("pm-is-supported",
"--suspend-hybrid", NULL);
> + *supported = canSuspend && canHibernate;
I'm not sure if your simpler checks will cause us to declare an action
unsupported on systems where it was previously declared supported by
pm-is-supported. I think the idea makes sense, but I'd like a second
opinion that we aren't hurting ourselves by doing fewer checks than what
we are replacing.
I'm not a power management expert either, so any other opinion is highly
appreciated. In the version I have here the hybrid case checks whether
/sys/class/rtc/rtc0/wakealarm is writeable.
--
Cedric
4:21 a.m.
On Fri, Mar 28, 2014 at 02:07:36PM -0600, Eric Blake wrote:
On 03/28/2014 10:32 AM, Cédric Bosdonnat wrote:
> From: Cédric Bosdonnat <cedric.bosdonnat(a)free.fr>
>
> pm-is-supported is the only thing needed in pm-utils, better get rid of
> it since systemd is heavily used for libvirt.
> ---
> src/util/virnodesuspend.c | 34 ++++++++++++++++++++--------------
> 1 file changed, 20 insertions(+), 14 deletions(-)
You also need to modify libvirt.spec.in to drop the dependency.
>
>
> + if (virFileReadAll("/sys/power/state", 1024, &buf) < 0)
> + goto cleanup;
> +
> + states = virStringSplit(buf, " ", 0);
> +
> + canSuspend = (virStringArrayHasString(states, "mem") ||
> + virStringArrayHasString(states, "standby"));
> + canHibernate = virStringArrayHasString(states, "disk");
pm-is-supported checks a bit more than what your replacement checks.
For suspend, it declares yes if any of these succeed:
grep -q mem /sys/power/state
[ -c /dev/pmu ] && pm-pmu --check
grep -q standby /sys/power/state
For hibernate, it requires that BOTH of these succeed:
[ -f /sys/power/disk ]
grep -q disk /sys/power/state
For hybrid, it requires that all three succeed:
[ -f /sys/power/disk ] && \
grep -q disk /sys/power/state && \
grep -q suspend /sys/power/disk
as well as having fallback code to fake a hybrid sleep by joining the
other two states.
What concerns me is that this is what it does today. We also need to
check whether it did anything different on older hosts like REL-5
vintage.
Regards,
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 :|
9:35 a.m.
On 03/28/2014 04:07 PM, Eric Blake wrote:
On 03/28/2014 10:32 AM, Cédric Bosdonnat wrote:
> From: Cédric Bosdonnat <cedric.bosdonnat(a)free.fr>
>
> pm-is-supported is the only thing needed in pm-utils, better get rid of
> it since systemd is heavily used for libvirt.
> ---
> src/util/virnodesuspend.c | 34 ++++++++++++++++++++--------------
> 1 file changed, 20 insertions(+), 14 deletions(-)
You also need to modify libvirt.spec.in to drop the dependency.
>
>
> + if (virFileReadAll("/sys/power/state", 1024, &buf) < 0)
> + goto cleanup;
> +
> + states = virStringSplit(buf, " ", 0);
> +
> + canSuspend = (virStringArrayHasString(states, "mem") ||
> + virStringArrayHasString(states, "standby"));
> + canHibernate = virStringArrayHasString(states, "disk");
pm-is-supported checks a bit more than what your replacement checks.
For suspend, it declares yes if any of these succeed:
grep -q mem /sys/power/state
[ -c /dev/pmu ] && pm-pmu --check
grep -q standby /sys/power/state
For hibernate, it requires that BOTH of these succeed:
[ -f /sys/power/disk ]
grep -q disk /sys/power/state
For hybrid, it requires that all three succeed:
[ -f /sys/power/disk ] && \
grep -q disk /sys/power/state && \
grep -q suspend /sys/power/disk
as well as having fallback code to fake a hybrid sleep by joining the
other two states.
> +
> switch (target) {
> case VIR_NODE_SUSPEND_TARGET_MEM:
> - cmd = virCommandNewArgList("pm-is-supported",
"--suspend", NULL);
> + *supported = canSuspend;
> break;
> case VIR_NODE_SUSPEND_TARGET_DISK:
> - cmd = virCommandNewArgList("pm-is-supported",
"--hibernate", NULL);
> + *supported = canHibernate;
> break;
> case VIR_NODE_SUSPEND_TARGET_HYBRID:
> - cmd = virCommandNewArgList("pm-is-supported",
"--suspend-hybrid", NULL);
> + *supported = canSuspend && canHibernate;
I'm not sure if your simpler checks will cause us to declare an action
unsupported on systems where it was previously declared supported by
pm-is-supported. I think the idea makes sense, but I'd like a second
opinion that we aren't hurting ourselves by doing fewer checks than what
we are replacing.
FWIW there's a fedora bug about dropping the libvirt dep:
https://bugzilla.redhat.com/show_bug.cgi?id=919390
And this:
https://www.redhat.com/archives/libvir-list/2013-August/msg00212.html
Which claims there's a logind replacement API. Since we aim to build on RHEL5
(where logind/systemd isn't available), maybe we should preserve this old
implementation and add a new one using the new APIs? Sufficiently new distros
just pass the needed configure flag in their build system.
- Cole
3841
days inactive
3844
days old
6 comments
6 participants
participants (6)
-
Cedric Bosdonnat -
Cole Robinson -
Cédric Bosdonnat
-
Daniel P. Berrange -
Doug Goldstein -
Eric Blake