On Wed, Dec 06, 2017 at 10:18:53AM -0500, John Ferlan wrote:
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?
The purpose of qemuDomain*DefValidate is to validate the domain definition, not the command line. So it still makes sense to check if what is provided in the domain definition matches the implicit device.
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.
Without this check, the IDE controller validation seemed a bit confusing, since it returned an error in all the cases, leaving up to the reader to figure out that the validate function is not called on valid devices. Also, since the pci(e)-root checks were skipped. Jan
+ */ +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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
