[libvirt] [PATCH REPOST 5/7] conf: Add support for virtio-scsi iothreads

[John Ferlan]

[...]

>>>> +    if (iothread) {
>>>> +        if (def->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI) {
>>>> +            virReportError(VIR_ERR_XML_ERROR,
>>>> +                           _("'iothread' attribute only supported for "
>>>> +                             "controller model '%s'"),
>>>> +                           virDomainControllerModelSCSITypeToString(VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI));
>>>> +            goto error;
>>>> +        }
>>>
>>> I think this particular check should be moved to one of the generic PostParse
>>> validation functions, since it's not specific to parse time.
>>>
>>
>> I guess I was under the impression that post parse callbacks were to be
>> used when we didn't have all the information about a relationship
>> between say a disk and controller.
>>
> 
> For driver specific bits, maybe. But in general I think as much validation as
> possible should move out of the XML parsing routines, and into the generic
> PostParse function, so drivers that manually populate virDomainDef hit the
> checks as well.
> 

OK, reasonable - I'll move the def->model part...

>> In this case, the attribute is only supported on the controller that has
>> been defined using "virtio-scsi" or "virtio-ccw".  Since those weren't
>> "discover-able" based on some other relationship, thus checking at parse
>> time was better.
>>
>> IDC either way, but I think (without too much digging), that would mean
>> a change to qemuDomainDeviceDefPostParse in the following if:
>>
> 
> I would put it in generic code instead. Roughly what
> virDomainDefPostParseTimer does, but probably tied into the device iteration.
> 
> Really the check being in done in DomainDefParse* isn't impactful in this
> case, but it's a pattern we should try to break out of IMO
> 

True - a lot easier to "decide" in the parse code where the check would
go considering other attributes that can be found with <driver> are
checked inline...  Chasing the post parse callbacks and deciding where
is the "best" place for a check just requires more thought.

I'll add a check virDomainDeviceDefPostParseInternal since it's a device
specific callback check as opposed to a domain def check.

I'll push 1-4 since they're ACK'd and repost 5-7 in a v2 shortly.

Tks -

John