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

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


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