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@xxxxxxx> >> >> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list