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