[v2,00/16] domcaps: use virTristateBool

Message ID cover.1551914794.git.crobinso@redhat.com
Headers show
Series
  • domcaps: use virTristateBool
Related show

Message

Cole Robinson March 6, 2019, 11:36 p.m.
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.

Again, bhyve changes are untested. If someone can give them a spin
that would be appreciated, otherwise I will try to get a freebsd build
setup.

Cole Robinson (16):
  tests: domcaps: Add a default 'empty' test
  tests: domcaps: Remove unused typedef
  tests: domcaps: Remove 'full' test
  conf: domcaps: Add single line formatting macro
  conf: domcaps: use virTristateBool for 'supported'
  qemu: domcaps: fill in explicit supported BOOL_NO
  libxl: domcaps: fill in explicit supported BOOL_NO
  bhyve: domcaps: fill in explicit supported BOOL_NO
  schemas: domcaps: Make more elements optional
  conf: domcaps: Don't output XML on tristate ABSENT
  conf: domcaps: Add virCapsEnum 'report'
  qemu: fill in virCapsEnum 'report'
  libxl: fill in virCapsEnum 'report'
  bhyve: fill in virCapsEnum 'report'
  conf: domcaps: Don't format XML on report=false
  docs: formatdomaincaps: Describe optional XML changes

 docs/formatdomaincaps.html.in        |  11 +++
 docs/schemas/domaincaps.rng          |  20 ++++-
 src/bhyve/bhyve_capabilities.c       |  27 ++++--
 src/conf/domain_capabilities.c       |  31 ++++---
 src/conf/domain_capabilities.h       |  21 ++---
 src/libxl/libxl_capabilities.c       |  31 +++++--
 src/qemu/qemu_capabilities.c         |  41 ++++++---
 tests/domaincapsschemadata/empty.xml |  16 ++++
 tests/domaincapsschemadata/full.xml  | 123 ---------------------------
 tests/domaincapstest.c               |  79 +----------------
 10 files changed, 155 insertions(+), 245 deletions(-)
 create mode 100644 tests/domaincapsschemadata/empty.xml
 delete mode 100644 tests/domaincapsschemadata/full.xml

-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Cole Robinson March 13, 2019, 2:38 p.m. | #1
On 3/6/19 6:36 PM, 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.

> 

> Again, bhyve changes are untested. If someone can give them a spin

> that would be appreciated, otherwise I will try to get a freebsd build

> setup.

> 


I have confirmed that after applying this series:

* bhyve builds and tests pass on freebsd 12.0
* runtime libxl domaincapabilities output on f29 does not change for 
both xenpv and xenfv

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Michal Privoznik March 18, 2019, 1:28 p.m. | #2
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?

Otherwise the patches look good.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson March 18, 2019, 1:53 p.m. | #3
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

Thanks,
Cole



- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Michal Privoznik March 18, 2019, 2:13 p.m. | #4
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list