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

On 11/29/2011 08:44 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > 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.

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.