[libvirt] [PATCH] Drop dependency on pm-is-supported
From: Cédric Bosdonnat <cedric.bosdonnat@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
On Mar 28, 2014, at 11:32 AM, Cédric Bosdonnat <cbosdonnat@suse.com> wrote:
From: Cédric Bosdonnat <cedric.bosdonnat@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@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
On 03/28/2014 01:49 PM, Doug Goldstein wrote:
On Mar 28, 2014, at 11:32 AM, Cédric Bosdonnat <cbosdonnat@suse.com> wrote:
From: Cédric Bosdonnat <cedric.bosdonnat@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
On 03/28/2014 10:32 AM, Cédric Bosdonnat wrote:
From: Cédric Bosdonnat <cedric.bosdonnat@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
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@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
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@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 :|
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@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
participants (6)
-
Cedric Bosdonnat -
Cole Robinson -
Cédric Bosdonnat
-
Daniel P. Berrange -
Doug Goldstein -
Eric Blake