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

[Pavel Hrdina]

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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190130/472da48e/attachment-0001.sig>