Re: [libvirt] [PATCH REPOST 5/7] conf: Add support for virtio-scsi iothreads
[...]
>>> + 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