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

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(a)redhat.com&gt; >>> --- >>> 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 toa 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.