[libvirt] [PATCH 10/14] Remove powerMgmt_valid field from capabilities struct

[Daniel P. Berrange]

On Tue, Nov 29, 2011 at 09:40:14AM -0700, Eric Blake wrote:
> On 11/29/2011 08:44 AM, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange at redhat.com>
> > 
> > If we ensure that virNodeSuspendGetTargetMask always resets
> > *bitmask to zero upon failure, there is no need for the
> > powerMgmt_valid field.
> > 
> > * src/util/virnodesuspend.c: Ensure *bitmask is zero upon
> >   failure
> > * src/conf/capabilities.c, src/conf/capabilities.h: Remove
> >   powerMgmt_valid field
> > * src/qemu/qemu_capabilities.c: Remove powerMgmt_valid
> 
> I'm not sure about this one.  This is a semantic change in the resulting
> XML.
> 
> > -    if (caps->host.powerMgmt_valid) {
> > -        /* The PM query was successful. */
> > -        if (caps->host.powerMgmt) {
> > -            /* The host supports some PM features. */
> > -            unsigned int pm = caps->host.powerMgmt;
> > -            virBufferAddLit(&xml, "    <power_management>\n");
> > -            while (pm) {
> > -                int bit = ffs(pm) - 1;
> > -                virBufferAsprintf(&xml, "      <%s/>\n",
> > -                    virCapsHostPMTargetTypeToString(bit));
> > -                pm &= ~(1U << bit);
> > -            }
> > -            virBufferAddLit(&xml, "    </power_management>\n");
> > -        } else {
> > -            /* The host does not support any PM feature. */
> > -            virBufferAddLit(&xml, "    <power_management/>\n");
> 
> Before, we had three cases (and documented them!):
> 
> no <power_management> in the XML - we could not query (or older server)
> 
> explicit <power_management/> - we successfully queried, but lack power
> management
> 
> explicit <power_management> with sub-elements - what features are supported
>
> 
> > +    /* The PM query was successful. */
> > +    if (caps->host.powerMgmt) {
> > +        /* The host supports some PM features. */
> > +        unsigned int pm = caps->host.powerMgmt;
> > +        virBufferAddLit(&xml, "    <power_management>\n");
> > +        while (pm) {
> > +            int bit = ffs(pm) - 1;
> > +            virBufferAsprintf(&xml, "      <%s/>\n",
> > +                              virCapsHostPMTargetTypeToString(bit));
> > +            pm &= ~(1U << bit);
> >          }
> > +        virBufferAddLit(&xml, "    </power_management>\n");
> > +    } else {
> > +        /* The host does not support any PM feature. */
> > +        virBufferAddLit(&xml, "    <power_management/>\n");
> 
> Whereas after the patch, we now _always_ output a form of
> <power_management>, thus collapsing 3 states into 2.
> 
> On the other hand, what is a user going to do about the difference
> between the first two?  There's nothing useful they can do by knowing
> whether a host lacks power management or whether libvirt lacked the
> ability to query power management; either way, it means they can't use
> power management API.

As you say its not really useful information, and IMHO we shouldn't
be exposing whether some internal operation failed or not, in the
XML schema.

> 
> So I'm 70-30 in favor of this patch.
> 
> At any rate, ACK to the code, but you also need to update
> docs/formatcaps.html.in to match, if we agree to go with it.

OK will update the docs

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