Re: [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&gt; >>> >>> 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 :-)