[libvirt] [PATCH 5/8] domcaps: Add function for initializing domain caps as unsupported

Wed Nov 20 18:02:04 UTC 2019

On 11/20/19 9:40 AM, Peter Krempa wrote:
> On Mon, Nov 18, 2019 at 14:43:08 -0500, Cole Robinson wrote:
>> On 11/13/19 11:05 AM, Peter Krempa wrote:
>>> For future extensions of the domain caps it's useful to have a single
>>> point that initializes all capabilities as unsupported by a driver. The
>>> driver then can enable specific ones.
>>>
>>> Signed-off-by: Peter Krempa <pkrempa at redhat.com>
>>> ---
>>>  src/bhyve/bhyve_capabilities.c |  4 +---
>>>  src/conf/domain_capabilities.c | 14 ++++++++++++++
>>>  src/conf/domain_capabilities.h |  2 ++
>>>  src/libvirt_private.syms       |  1 +
>>>  src/libxl/libxl_capabilities.c |  5 ++---
>>>  5 files changed, 20 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c
>>> index c04a475375..f80cf7be62 100644
>>> --- a/src/bhyve/bhyve_capabilities.c
>>> +++ b/src/bhyve/bhyve_capabilities.c
>>> @@ -116,9 +116,7 @@ virBhyveDomainCapsFill(virDomainCapsPtr caps,
>>>      }
>>>
>>>      caps->hostdev.supported = VIR_TRISTATE_BOOL_NO;
>>> -    caps->iothreads = VIR_TRISTATE_BOOL_NO;
>>> -    caps->vmcoreinfo = VIR_TRISTATE_BOOL_NO;
>>> -    caps->genid = VIR_TRISTATE_BOOL_NO;
>>> +    virDomainCapsFeaturesInitUnsupported(caps);
>>>      caps->gic.supported = VIR_TRISTATE_BOOL_NO;
>>>
>>>      return 0;
>>> diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
>>> index 8d0a0c121c..39acad00f1 100644
>>> --- a/src/conf/domain_capabilities.c
>>> +++ b/src/conf/domain_capabilities.c
>>> @@ -316,6 +316,20 @@ virDomainCapsEnumClear(virDomainCapsEnumPtr capsEnum)
>>>  }
>>>
>>>
>>> +/**
>>> + * @caps: domain caps
>>> + *
>>> + * Initializes all features in 'caps' as unsupported.
>>> + */
>>> +void
>>> +virDomainCapsFeaturesInitUnsupported(virDomainCapsPtr caps)
>>> +{
>>> +    caps->iothreads = VIR_TRISTATE_BOOL_NO;
>>> +    caps->vmcoreinfo = VIR_TRISTATE_BOOL_NO;
>>> +    caps->genid = VIR_TRISTATE_BOOL_NO;
>>> +}
>>> +
>>> +
>>
>> This pattern is suboptimal IMO. Any time a new domcaps capability is
>> added, say to serve the qemu/ driver, the developer now needs to
>> consider how this shared function impacts libxl domcaps XML.
> 
> I see your point, but I think there's merit also towards the other
> approach.
> 
> Here we explicitly mark everything as unsupported, but that does not
> prevent developers from resetting the caps to the missing state in case
> when it's ambiguous or they don't wish to deal with other hypervisors.
> 
>>
>> Say hypothetically tomorrow I want to expose 'acpi' support in domcaps
>> <features>. My main goal is to expose this in qemu domcaps. Naturally a
>> starting point is to add 'caps->acpi = VIR_TRISTATE_BOOL_NO;' here.
>>
>> If I don't remember to check libxl code though, I've now potentially
>> converted their driver to report blatantly incorrect domcaps output, as
>> libxl does support ACPI. If I do remember to check their code, it's my
>> responsibility to edit libxl code to ensure it's reporting accurate
>> information, which makes my life harder.
> 
> In my opinion it's not wrong to ask developers to at least think whether
> the change does not influence other hypervisors as well or in case when
> it's trivially supported to more than the original intention.
> 

I agree with that statement. Devs should be considering other drivers
when extending domcaps schema. That doesn't change with the current way
or the previous way. What changes is the default result if other drivers
are not considered properly:

* old way:
** libxl/bhyve driver is ignored: their XML output doesn't change at
all. missed opportunity to improve their XML output coverage, but
otherwise the status quo is maintained
** libxl/bhyve driver is considered: requires explicit changes in their
code to support supported='no' or supported='yes' case.

* git way:
** libxl/bhyve driver is ignored.
*** 1) supported='no' is correct state. only likely result is CI
breakage or review issues with bhyve caps needing regenerating
*** 2) supported='no' is incorrect state. unless review catches this,
libxl/bhyve now may be reporting blatantly incorrect info.
** libxl/bhyve driver is considered: supported='no' requires no code
change, but supported='yes' requires some code change

>From my perspective as an app developer, here's my order of worst to
best cases for domcaps output:

1) domcaps output is present but incorrect
2) domcaps output is absent
3) domcaps output is present and correct

And there's a giant distance between 1 and 2. #2 is basically the
default domcaps state for a whole bunch of interesting information, apps
already can't depend on it existing because it's not provided by any driver.

The case we really should be optimizing for IMO is avoiding #1, and the
current git code makes it easier to introduce errors like that.

I admit, it's kind of a philosophical argument for the <features> XML. I
expect most things that features are extended for will only count for
the qemu driver. But if this pattern were to be extended to things like
<devices>, essentially the old state the code was in before my patches
earlier in the year, it becomes particularly dangerous. Take the <sound>
example I gave in the cover letter of my series.

> In the end they still can remove it in case it's out of the scope.
> 
> Also we should take into account the usability of the caps themselves.
> If we by default promote bitrot of other hypervisor drivers we might
> miss out on stuff that is actually supported but nobody bothered to wire
> up the capability for it.
> 
>> With the previous behavior, I could add a new domcap feature to central
>> code, fill it in for qemu, and libxl output wouldn't change at all.
>> Whether to report supported='no' or supported='yes' is left entirely up
>> to libxl driver code. This makes it easier and safer to extend domcaps IMO.
>>
>> This was the goal of my series to use tristate bool earlier in the year,
>> where there's more explanation:
>> https://www.redhat.com/archives/libvir-list/2019-March/msg00365.html
> 
> I fully agree that we should keep it tristate where possible but also
> the most common option will be that the capability is not supported by
> other hypervisors when implementing it.
> 

Yes for new capabilities this is true. But extending domcaps to expose
existing XML support values (the acpi bit for example) is less clear

> In the end neither approach will get rid of the necessity to do proper
> review, but keeping the caps hidden promotes ignorance because the tests
> don't catch the possible scope of change.
> In the end, we can revert this behaviour (well, it will require some
> tweaking now not a straight revert) if you disagree with my reasoning.
> 

I see your points but I still favor the old way. There's no rush to
change this IMO so maybe we can come up with consensus on whether it's
optimal to put default supported=yes|no values in common code or not.
CCing Jano and Michal for their opinions too.

Thanks,
Cole