[libvirt] [PATCH v2 00/16] domcaps: use virTristateBool

[Michal Privoznik]

On 3/18/19 2:53 PM, Cole Robinson wrote:
> On 3/18/19 9:28 AM, Michal Privoznik wrote:
>> On 3/7/19 12:36 AM, Cole Robinson wrote:
>>> v1 posting: 
>>> https://www.redhat.com/archives/libvir-list/2019-February/msg01088.html
>>>
>>> v2 changes:
>>>      - Rebase to master
>>>      - Remove the full.xml test in patch #3
>>>      - Add virCapsEnum 'format' and use it
>>>      - Extend docs to explain optional XML
>>>
>>> v1 cover letter:
>>> Extending domaincapabilities with new XML schema is currently a bit of
>>> a maintenance pain. Consider the case of adding a new enum for listing
>>> <sound> models. I want to output this info for the qemu driver.
>>>
>>> Internally in the domaincapabilities plumbing, whether a <device> is
>>> supported= is tracked with boolean true/false. If I extend that
>>> pattern for <sound> devices and fill in data for the qemu driver, the
>>> other domcaps implementations will now automatically output a new XML
>>> element:
>>>
>>>    <sound supported='no'>
>>>
>>> Now, for bhyve I can 'git grep' confirm that it doesn't have any
>>> <sound> support, but for xen/libxl it _is_ supported. So if I don't
>>> fill in accurate support in the xen driver, I've just made their
>>> domcaps report blatantly incorrect info.
>>>
>>> Ideally I would make these <sound> changes and the other drivers output
>>> would _not_ change. xen output would now be incomplete, but not
>>> obviously wrong, which is easier on me the developer, and safer for the
>>> API consumer.
>>>
>>> This moves domcaps plumbing in that direction. It switches most
>>> internal 'supported' fields to virTristateBool so we can track an
>>> ABSENT state and map that to outputting no XML. Explicit supported='no'
>>> values are filled in where needed to ensure existing driver XML doesn't
>>> change. cpu and sev supported= values are left unconverted, but they
>>> require semi-special handling anyways so aren't really affected by the
>>> problem I laid out above.
>>>
>>>
>>> In v2, I additionally added a mechanism to make <enum> values optionally
>>> formatted. Right now whenever a new <enum> is added, if the parent bit
>>> is supported (like <disk supported='yes'/>), the new <enum> is
>>> automatically formatted as well. This has the same problem described
>>> above with the @supported bit. Now drivers are required to set a
>>> virCapsPtr.report = true if they want the <enum> to be formatted.
>>> Existing drives have this value filled in to maintain back compat.
>>
>> What if we'd have virDomainCapsEnumSet() also set enum.report = true? 
>> That should still work as if a driver doesn't support given enum it 
>> won't call the function, would it?
>>
> 
> I thought about this case. The downside is in an example like 
> virQEMUCapsFillDomainDeviceVideoCaps. Every video model we report is 
> dependent on a QEMU_CAPS check. If though a qemu binary doesn't have any 
> of those devices, we want to report an empty enum to reflect that. If we 
> implicitly depend on setting 'report = true' via 
> VIR_DOMAIN_CAPS_ENUM_SET, then we won't get the empty enum in this case, 
> instead report no enum at all.
> 
> Keeping the 'report' bit explicit makes it harder to accidentally 
> introduce an issue like that. It's a corner case but that was the 
> motivation
> 

Fair enough.

ACK series.

Michal