Re: [libvirt] [PATCH 5/8] domcaps: Add function for initializing domain caps as unsupported
On 11/20/19 7:02 PM, Cole Robinson wrote: 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. I prefer independent domcaps among drivers. It's easier to expose new capability just for one driver (just like Cole demonstrates in his hypothetical example). It would be nice to have rich and updated domcaps for other drivers too but looks like there is not enough interest/manpower/.. to do that. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/8] domcaps: Add function for initializing domain caps as unsupported
On Wed, Nov 20, 2019 at 13:02:04 -0500, Cole Robinson wrote: > 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 > >>> --- > >>> 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 > >> . 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
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 >>> --- >>> 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 >> . 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 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
Re: [libvirt] [PATCH 5/8] domcaps: Add function for initializing domain caps as unsupported
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 > > --- > > 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 > . 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. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 > --- > 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 . 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 Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/8] domcaps: Add function for initializing domain caps as unsupported
On Wed, Nov 13, 2019 at 05:05:23PM +0100, 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 --- 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(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/8] domcaps: Add function for initializing domain caps as unsupported
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 --- 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; +} + + static int virDomainCapsEnumFormat(virBufferPtr buf, const virDomainCapsEnum *capsEnum, diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index 6b27eac11f..9baaea8f60 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -226,6 +226,8 @@ int virDomainCapsEnumSet(virDomainCapsEnumPtr capsEnum, unsigned int *values); void virDomainCapsEnumClear(virDomainCapsEnumPtr capsEnum); +void virDomainCapsFeaturesInitUnsupported(virDomainCapsPtr caps); + char * virDomainCapsFormat(const virDomainCaps *caps); int virDomainCapsDeviceDefValidate(const virDomainCaps *caps, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4d0d03c580..1432f1697a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -202,6 +202,7 @@ virDomainCapsCPUUsableTypeToString; virDomainCapsDeviceDefValidate; virDomainCapsEnumClear; virDomainCapsEnumSet; +virDomainCapsFeaturesInitUnsupported; virDomainCapsFormat; virDomainCapsNew; virSEVCapabilitiesFree; diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c index fe792e9a82..55f6b490ec 100644 --- a/src/libxl/libxl_capabilities.c +++ b/src/libxl/libxl_capabilities.c @@ -764,9 +764,8 @@ libxlMakeDomainCapabilities(virDomainCapsPtr domCaps, libxlMakeDomainDeviceHostdevCaps(hostdev) < 0) return -1; -domCaps->iothreads = VIR_TRISTATE_BOOL_NO; -domCaps->vmcoreinfo = VIR_TRISTATE_BOOL_NO; -domCaps->genid = VIR_TRISTATE_BOOL_NO; +virDomainCapsFeaturesInitUnsupported(domCaps); + domCaps->gic.supported = VIR_TRISTATE_BOOL_NO; return 0; -- 2.23.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list