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

[Peter Krempa]

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.

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.

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20191120/c49483a3/attachment-0001.sig>