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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux