[00/10] domcaps: use virTristateBool

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

Message

Cole Robinson Feb. 19, 2019, 8:09 p.m.
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.

Cole Robinson (10):
  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

 docs/schemas/domaincaps.rng          | 20 +++++--
 src/bhyve/bhyve_capabilities.c       | 20 +++++--
 src/conf/domain_capabilities.c       | 26 ++++++----
 src/conf/domain_capabilities.h       | 20 +++----
 src/libxl/libxl_capabilities.c       | 18 ++++---
 src/qemu/qemu_capabilities.c         | 26 ++++++----
 tests/domaincapsschemadata/empty.xml | 16 ++++++
 tests/domaincapstest.c               | 78 ++--------------------------
 8 files changed, 103 insertions(+), 121 deletions(-)
 create mode 100644 tests/domaincapsschemadata/empty.xml

-- 
2.20.1

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

Comments

John Ferlan Feb. 25, 2019, 4:33 p.m. | #1
On 2/19/19 3:09 PM, Cole Robinson wrote:
> 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.

> 

> Cole Robinson (10):

>   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

> 

>  docs/schemas/domaincaps.rng          | 20 +++++--

>  src/bhyve/bhyve_capabilities.c       | 20 +++++--

>  src/conf/domain_capabilities.c       | 26 ++++++----

>  src/conf/domain_capabilities.h       | 20 +++----

>  src/libxl/libxl_capabilities.c       | 18 ++++---

>  src/qemu/qemu_capabilities.c         | 26 ++++++----

>  tests/domaincapsschemadata/empty.xml | 16 ++++++

>  tests/domaincapstest.c               | 78 ++--------------------------

>  8 files changed, 103 insertions(+), 121 deletions(-)

>  create mode 100644 tests/domaincapsschemadata/empty.xml

> 


I think in patch3, you probably should remove the full.xml file too.

Logically, it seems things work; however, I am curious what happens
if/when this is applied to bhvye and libxl environments. I think it
would be good to get someone to apply this there and be sure there's no
unexpected (for you) failures.

Interesting "view" for <cpu> on "empty.xml" - digging deeper shows that
"mode" could be optional (at least that's how I read the formatdomain
text). So rather than "no" for all 3, there would be nothing.

Similarly for SEV the "no" just is some default/optional value matching
your RNG changes.  Also, even though it wouldn't necessarily be a
"feature" of perhaps Intel, someone running on AMD could I assume get a
different result than you got. IOW: Same problem I ran into with that 2
patch series trying to "fake" SEV output.

If cpu, devices, and features have no subelements - should they even be
displayed?  Leaving purely just the path, domain, machine, and arch output?

Even with all that - is there any thought that perhaps some application
has made use of the existing "functionality" to spit out "no" for those
undefined/missing (e.g. ABSENT) values.  IOW: It seems via the RNG we
would have made claims that certain output exists that we're now making
optional. I conceptually don't have an issue with it, I'm just thinking
terms of what API output contract we may have made.  Yes, those
applications should be able to handle missing fields, but at the very
least we probably would need to update the news.xml to let the consumer
know.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson March 6, 2019, 10:19 p.m. | #2
On 2/25/19 11:33 AM, John Ferlan wrote:
> 

> 

> On 2/19/19 3:09 PM, Cole Robinson wrote:

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

>>

>> Cole Robinson (10):

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

>>

>>  docs/schemas/domaincaps.rng          | 20 +++++--

>>  src/bhyve/bhyve_capabilities.c       | 20 +++++--

>>  src/conf/domain_capabilities.c       | 26 ++++++----

>>  src/conf/domain_capabilities.h       | 20 +++----

>>  src/libxl/libxl_capabilities.c       | 18 ++++---

>>  src/qemu/qemu_capabilities.c         | 26 ++++++----

>>  tests/domaincapsschemadata/empty.xml | 16 ++++++

>>  tests/domaincapstest.c               | 78 ++--------------------------

>>  8 files changed, 103 insertions(+), 121 deletions(-)

>>  create mode 100644 tests/domaincapsschemadata/empty.xml

>>

> 

> I think in patch3, you probably should remove the full.xml file too.

> 

> Logically, it seems things work; however, I am curious what happens

> if/when this is applied to bhvye and libxl environments. I think it

> would be good to get someone to apply this there and be sure there's no

> unexpected (for you) failures.

> 

> Interesting "view" for <cpu> on "empty.xml" - digging deeper shows that

> "mode" could be optional (at least that's how I read the formatdomain

> text). So rather than "no" for all 3, there would be nothing.

> 

> Similarly for SEV the "no" just is some default/optional value matching

> your RNG changes.  Also, even though it wouldn't necessarily be a

> "feature" of perhaps Intel, someone running on AMD could I assume get a

> different result than you got. IOW: Same problem I ran into with that 2

> patch series trying to "fake" SEV output.

> 


Yes, cpu and sev bits would ideally get similar treatment, that's an
idea for a follow up series for sure.

> If cpu, devices, and features have no subelements - should they even be

> displayed?  Leaving purely just the path, domain, machine, and arch output?

> 


Would certainly make for nicer XML. Not a priority for me in the near
term though. FWIW my goal for the short/medium term with this stuff is
making it as simple as possible to extend domaincapabilities with the
multitude of data it is lacking.

> Even with all that - is there any thought that perhaps some application

> has made use of the existing "functionality" to spit out "no" for those

> undefined/missing (e.g. ABSENT) values.  IOW: It seems via the RNG we

> would have made claims that certain output exists that we're now making

> optional. I conceptually don't have an issue with it, I'm just thinking

> terms of what API output contract we may have made.  Yes, those

> applications should be able to handle missing fields, but at the very

> least we probably would need to update the news.xml to let the consumer

> know.


It's not impossible I suppose. But if you parse the XML like that then
you are already restricting your ability to handle older libvirt domcaps
XML which may not output the specific field at all: think before <sev>
bits were added. I think in general with libvirt if you are parsing XML
such that you are always expecting a value to be present, you are doing
it wrong, because almost every XML bit in the domain schema at least is
optionally missing.

Regardless I can extend the docs to try and clarify how I think the API
should be consumed:

"Some XML elements may be entirely omitted from the domaincapabilities
XML, depending on what the libvirt driver has filled in. Applications
should only act on what is explicitly reported in the domaincapabilities
XML. For example, if <disk supported='yes'/> is present, you can safely
assume the driver supports <disk> devices. If <disk supported='no'/> is
present, you can safely assume the driver does NOT support <disk>
devices. If the <disk> block is omitted entirely, the driver is not
indicating one way or the other whether it supports <disk> devices, and
applications should not interpret the missing block to mean any thing in
particular."

Thanks,
Cole

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