On Tue, Jan 29, 2019 at 04:32:09PM +0100, Andrea Bolognani wrote:
> 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.
If we add model='virtio' we should always translate it back to
'virtio-scsi'. It's not a new model or new feature, it's just a
different name for existing model and we should not break management
applications that are already using 'virtio-scsi'. It would be
basically only alias. The question is whether it's useful, if
management application starts using 'virtio' when creating new guest it
would still had to be able to parse 'virtio-scsi' and my guess is that
it would not help at all.
Pavel
>
> [...]
> > @@ -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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
