[libvirt] [PATCH] Drop dependency on pm-is-supported

[Cedric Bosdonnat]

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 at 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