Re: Re: [PATCH 20/33] qemu: Enhance qemuDomainDefaultUSBControllerModel()

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.

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.