[...]
>>>> + 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list