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

[Ján Tomko]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20171206/c859dc1f/attachment-0001.sig>