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 <pkrempa@xxxxxxxxxx>
> >>> ---
> >>> 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.
You can't prevent any of the case by pure code. You need to have review.
As a reviewer you are far more likely to notice something that is
added by a patch is wrong rather than something which is omitted in a
patch but should be there.
In my opinion the tradeoff between missing a wrong value reported in the
caps during review and missing that we could do better in other areas as
well was of comparable value despite the distance between #1 and #2.
Unfortunately I don't see any better way ensuring that other drivers are
considered beyond this. Adding a hacks to force a compile time check as
we did in far more important cases [1] definitely is not worth in this
scenario.
[1] commit 5e3c3447857faeada02702d5b186a6c805858a74
> 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
I still think it's always worth considering all drivers even when adding
capabilities for existing XML bits. Also gradually adding caps for
existing XML will become less and less prominent as APPs already assume
many supported bits from other places since our caps are far from
complete and actually we should promote adding detectability more and
more. We do a terrible job with it currently.
Ideally we would do it implicitly as qemu does because any non-implicit
way will be forgotten. Even in qemu if a patch requires to be detected
it requires manual addition to the schema which is not done by default.
> > 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.
Fair enough. As I don't see a way to force developers considering other
drivers which would be worth adding besides this one I'll prepare the
patch and the conversation can be done there.
Also honestly I'd not notice that bhyve would be changed with it and in
the end changing this to the old way will probably result in me not
adding the 'backingStoreInput' to bhyve as I don't feel like running
freebsd to gather the results.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
