On Fri, Sep 19, 2025 at 07:26:21 -0700, Andrea Bolognani wrote: > On Thu, Sep 18, 2025 at 03:57:10PM +0200, Peter Krempa wrote: > > On Tue, Aug 19, 2025 at 18:22:28 +0200, Andrea Bolognani via Devel wrote: > > > Extract the logic from qemuDomainControllerDefPostParse() to > > > a dedicated helper. The behavior is unchanged. > > > > I don't see it changed anywhere else ... is it just to separate stuff? > > I'm not sure I understand the question. > > Yes, this commit is intended to merely factor out the existing code > into a dedicated helper, without altering the behavior. Then later > commits perform the modifications we want.
I was asking for the reason for exporting it since it's used in single place. Especially since you put it in a different module so it needs to be exported. Extracting is fine, but extracting to a different module is a bit weird if you don't reuse it. > > > +/** > > > + * qemuDomainDefaultUSBControllerModel: > > > + * @def: domain definition > > > + * @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. > > > > This statement is a bit misleading. In cases where qemuCaps is NULL you > > must not be doing any decisions that depend on the caps that wouldn't be > > undone by another run of this function with caps present. > > > > In cases where the caps for given qemu are not present and the XML > > parsed is one of the persistent configs loaded from disk, the postparse > > code will be re-tried at next startup attempt where it has to be present > > for the VM to work. > > > > If you'd pick an incorrect model based on missing caps that'd be > > remembered and not fixed on startup, but it would behave differently if > > the caps are present. > > Got it. In practice things should already work correctly with the > current code, and continue to do so even after my patches, since > neither this helper nor its AutoAdded variant are ever called when > qemuCaps is NULL. > > Based on this and the fact that you provided a R-b for the patch, it > should be enough for me to update the comment so that it's not > misleading, right? Basically s/, if provided, might/will/g Yes, updating the comment is sufficient. It needs to mention that 'qemuCaps' *might* be NULL, in which case it must not try to infer any information that something is *not* supported because it actually may be, thus must pick the default that makes us re-check.
