On Thu, Jan 25, 2024 at 10:59:29 -0800, Andrea Bolognani wrote:
> On Thu, Jan 25, 2024 at 03:00:10PM +0100, Peter Krempa wrote:
> > On Wed, Jan 24, 2024 at 20:37:40 +0100, Andrea Bolognani wrote:
> > > +/**
> > > + * qemuDomainDefaultUSBControllerModel:
> > > + * @def: domain definition
> > > + * @cont: controller definition, or NULL
> > > + * @autoAdded: whether this controller is being automatically added
> > > + * @qemuCaps: QEMU capabilities, or NULL
> > > + * @parseFlags: parse flags
> > > + *
> > > + * Choose a reasonable model to use for a USB controller where a
> > > + * specific one hasn't been provided by the user.
> > > + *
> > > + * The choice is based on a number of factors, including the guest's
> > > + * architecture and machine type. @qemuCaps, if provided, might be
> > > + * taken into consideration too.
> > > + *
> > > + * @autoAdded should be true is this is a controller that libvirt is
> > > + * trying to automatically add on domain creation for the user's
> > > + * convenience: in that case, the function might decide to simply not
> > > + * add the controller instead of reporting a failure.
> > > + *
> > > + * Returns: >=0 (a virDomainControllerModelUSB value) on success,
> > > + * -1 on error, and
> >
> > This is NOT an error and is misrepresenting the _DEFAULT case which has
> > -1, which is also a success case at least in some situations.
>
> Fair enough.
>
> > I also don't think that this function needs to know
> > whether the controller was auto-added, or not
>
> It does though. For q35, we need to handle two cases:
>
> 1. a USB controller was present in the input XML, with no model
> provided;
>
> 2. a USB controller was NOT present in the input XML, but we're
> trying to add one by default.
>
> The outcome is different, as can be seen by comparing the output
> files for the relevant default-models and minimal tests:
>
> 1. the controller will be piix3-uhci;
>
> 2. the controller will be qemu-xhci.
>
> To complicate things further, the behavior is also different if the
> necessary devices are not available in the QEMU binary:
>
> 1. an error will be reported;
>
> 2. the controller will simply not be added.
>
> Personally I think that this is borderline madness, but it is the
> current behavior and with this series I'm only trying to reorganize
> things, not change them, at least until the last few patches.
>
> So, the additional argument is necessary in order to know which one
> of the two behaviors is desired/expected by the caller.
Oh, that's indeed madness. I didn't realize originally that there's a
difference between auto-selecting a model for an existing controller and
auto-selecting a model for an auto-added controller.
This obviously makes sense in the extent of the existing logic.
> Of course we didn't have this problem when we had two distinct chunks
> of code dealing with this default, but that came with its own issues
> and overall this approach seems preferable to me.
>
> > > + * -2 if no suitable model could be found but it's okay to
> > > + * just skip the controller altogether.
> >
> > IMO this should be VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE and not a new
> > arbitrary value.
>
> Technically, yeah, that would work. But mostly by chance.
>
> Right now the -2 return value is only treated differently in the
> context of the auto-added USB controller, which is something that we
> don't do for s390x guests. But for those, NONE is a valid return
> value that needs to be handled by the other caller of the helper...
>
> Making the two overlap would IMO make the code more fragile against
> future changes, however unlikely they might be.
>
> And yes, that's another piece of borderline madness that however we
> have to preserve :)
>
> > > @@ -5695,7 +5733,7 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
> > >
> > > case VIR_DOMAIN_CONTROLLER_TYPE_USB:
> > > if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT && qemuCaps) {
> > > - cont->model = qemuDomainDefaultUSBControllerModel(def, cont, qemuCaps, parseFlags);
> > > + cont->model = qemuDomainDefaultUSBControllerModel(def, cont, false, qemuCaps, parseFlags);
> >
> > The code can check here explicitly whether _NONE was returned and
> > report appropriate error.
>
> This is exactly the place where we need to make sure that we do *not*
> consider NONE an error, because of s390x.
As long as you properly document the return values and mention the
actual enum value name in addition to -1:
Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
