[...]
+ 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