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

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.

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.

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