Re: [libvirt] [PATCH 02/17] qemu: Introduce qemuDomainDeviceDefSkipController

On 12/05/2017 09:18 AM, Ján Tomko wrote:

> > We should not skip validation for implicit devices. Some of the checks > added later are for implicit devices (PCI root, SATA controller). > > It would be enough to split out the logic of figuring out whether the > controller is implicit out of command line generation. > After some more thought - I'm not sure it's really clear to me what you're driving at. The whole point of the Skip helper was to avoid duplicating checks in specific ValidateController{IDE|PCI|SATA|USB} helpers. If we're not building a command line for those, then what is the purpose of going through Validation?

I suppose I could duplicate the same checks in the helpers, but that didn't feel right. If I take the IDE check as an example, the IDE validation would need code like Lin Ma's patches had, e.g.: if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) { /* No need to check implicit/builtin IDE controller */ if (controller->idx == 0 && qemuDomainHasBuiltinIDE(def)) return 0; ... Personally, trying to reduce cut-copy-paste would be a gain.

>> + */ >> +bool >> +qemuDomainDeviceDefSkipController(const virDomainControllerDef >> *controller, >> +                                  const virDomainDef *def, >> +                                  unsigned int *flags) >> +{ >> +    /* skip USB controllers with type none.*/ >> +    if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && >> +        controller->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) { >> +        *flags |= QEMU_DOMAIN_DEVICE_SKIP_USB_CONTROLLER_FLAG; >> +        return true; >> +    } > > Simply returning true here (no USB controller is implicit) should > suffice. The caller can figure out whether USB_NONE is present > by going through the controllers array again (via virDomainDefHasUSB). > I'm just going to drop moving the USB checks into ValidateControllerUSB - it seems you have an idea of what you'd like to see done and I'm fine with reviewing that when/if it shows up on the list. The whole reason for the flags argument was because of the very specific USB checks being done in command line building. Since you clearly understand those better than I do, for me to make progress it's best to just defer to you. Updated series to be sent shortly... John