Re: [libvirt] [PATCH v2 24/25] qemu: Support scsi controller model=virtio-{non-}transitional

On Wed, 2019-01-23 at 16:32 -0500, Cole Robinson wrote: [...] > +++ b/docs/schemas/domaincommon.rng > @@ -2153,6 +2153,8 @@ > <value>ibmvscsi</value> > <value>virtio-scsi</value> > <value>lsisas1078</value> > + <value>virtio-transitional</value> > + <value>virtio-non-transitional</value> As mentioned during the previous round of reviews, I think we should support model='virtio' (which would behave the same as the existing model='virtio-scsi') in order to have a nice, consistent experience for users and management application developers.

[...] > @@ -4859,11 +4862,12 @@ virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr dev, > virDomainControllerDefPtr cdev = dev->data.controller; > > if (cdev->iothread && > - cdev->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI) { > - virReportError(VIR_ERR_XML_ERROR, > + cdev->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI && > + cdev->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL && > + cdev->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > _("'iothread' attribute only supported for " > - "controller model '%s'"), > - virDomainControllerModelSCSITypeToString(VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI)); > + "virtio scsi controllers")); > return -1; > } You could also use virReportError(VIR_ERR_XML_ERROR, _("'iothread' attribute not supported for " "controller model '%s'"), virDomainControllerModelSCSITypeToString(cdev->model)); I usually prefer that pattern, but either way works. A more interesting change would be to move this check... [...] > qemuDomainDeviceDefValidateControllerAttributes(const virDomainControllerDef *controller) > { > if (!(controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI && > - controller->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI)) { > + (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI || > + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL || > + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL))) { > if (controller->queues) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > _("'queues' is only supported by virtio-scsi controller")); ... here in a small preparatory patch, which will also lead to adding the new models to one less location in this one. But again, that's just bikeshedding and either way is fine :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list