[libvirt] [PATCH 16/18] qemu: Support scsi controller model=virtio-{non-}transitional

[Andrea Bolognani]

On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:
> Add <controller type='scsi' model handling for virtio transitional
> devices. Ex:
> 
>   <controller type='scsi' model='virtio-transitional'/>
> 
> * "virtio-transitional" maps to qemu "virtio-scsi-pci-transitional"
> * "virtio-non-transitional" maps to qemu "virtio-scsi-non-transitional"
> 
> The naming here doesn't match the pre-existing model=virtio-scsi.
> The prescence of '-scsi' there seems kind of redundant as we have
> type='scsi' already, so I decided to follow the pattern of other
> patches and use virtio-transitional etc.

Completely agree with the rationale here; however, in order to
really match the way other devices are handled, I think we should
make it so you can use

  <controller type='scsi' model='virtio'/>

as well, which of course would behave the same as the currently
available version. What do you think?

[...]
> +++ b/src/conf/domain_conf.c
> @@ -359,7 +359,9 @@ VIR_ENUM_IMPL(virDomainControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAS
>                "vmpvscsi",
>                "ibmvscsi",
>                "virtio-scsi",
> -              "lsisas1078");
> +              "lsisas1078",
> +              "virtio-transitional",
> +              "virtio-non-transitional");

Same comment as always for VIR_ENUM_IMPL().

[...]
> @@ -1146,6 +1148,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
>      {"vhost-vsock-pci-non-transitional", QEMU_CAPS_DEVICE_VHOST_VSOCK_NON_TRANSITIONAL},
>      {"virtio-input-host-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_TRANSITIONAL},
>      {"virtio-input-host-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_NON_TRANSITIONAL},
> +    {"virtio-scsi-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_SCSI_TRANSITIONAL},
> +    {"virtio-scsi-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_SCSI_NON_TRANSITIONAL},
>  };

Same comment as always for capabilities.

[...]
> @@ -507,12 +507,20 @@ qemuBuildVirtioTransitional(virBufferPtr buf,
>              tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_TRANSITIONAL;
>              ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_NON_TRANSITIONAL;
>              break;

Empty line here.

> +        case VIR_DOMAIN_DEVICE_CONTROLLER:
> +            if (strstr(baseName, "scsi")) {
> +                has_tmodel = model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL;
> +                has_ntmodel = model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL;
> +                tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_SCSI_TRANSITIONAL;
> +                ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_SCSI_NON_TRANSITIONAL;
> +                break;
> +            }
> +            return 0;

Using strstr() here is kinda crude, especially since the caller has
enough information to pass the appropriate virDomainControllerType
value, both in this case and later on for serial controllers.

I'd say just add yet another argument to the function. Yeah, it
starts to get quite unsightly, but I'd really rather not resort to
string comparison when a nice, type safe enum will do.

[...]
> +++ b/tests/qemuxml2xmltest.c
> @@ -1271,14 +1271,16 @@ mymain(void)
>              QEMU_CAPS_DEVICE_PCIE_ROOT_PORT,
>              QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY,
>              QEMU_CAPS_DEVICE_VHOST_VSOCK,
> -            QEMU_CAPS_VIRTIO_INPUT_HOST);
> +            QEMU_CAPS_VIRTIO_INPUT_HOST,
> +            QEMU_CAPS_VIRTIO_SCSI);
>      DO_TEST("virtio-non-transitional",
>              QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
>              QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE,
>              QEMU_CAPS_DEVICE_PCIE_ROOT_PORT,
>              QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY,
>              QEMU_CAPS_DEVICE_VHOST_VSOCK,
> -            QEMU_CAPS_VIRTIO_INPUT_HOST);
> +            QEMU_CAPS_VIRTIO_INPUT_HOST,
> +            QEMU_CAPS_VIRTIO_SCSI);

This too could go in patch 2/18.

-- 
Andrea Bolognani / Red Hat / Virtualization