[libvirt] [PATCH v2 7/7] qemu: Unify generation of command line for virtio devices

[Ján Tomko]

On Thu, Sep 06, 2018 at 02:22:19PM +0200, Andrea Bolognani wrote:
>A virtio device such as
>
>  <controller type='scsi' model='virtio-scsi'/>
>
>will be translated to one of four different QEMU devices
>based on the address type. This behavior is the same for
>all virtio devices, but unfortunately we have separate
>ad-hoc code dealing with each and every one of them: not
>only this is pointless duplication, but it turns out
>that most of that code is not robust against new address
>types being introduced and some of it is outright buggy.
>
>Introduce a new function, qemuBuildVirtioDevStr(), which
>deals with the issue in a generic fashion, and rewrite
>all existing code to use it.
>
>This fixes a bunch of issues such as virtio-serial-pci
>being used with virtio-mmio addresses and virtio-gpu
>not being usable at all with virtio-mmio addresses.
>
>It also introduces a couple of minor regressions,
>namely no longer erroring out when attempting to
>use virtio-balloon and virtio-input devices with
>virtio-s390 addresses; that said, virtio-s390 has
>been superseded by virtio-ccw such a long time ago
>that recent QEMU releases have dropped support for
>the former entirely,

I approve of such disregard for antique virtio.

>so re-implementing such
>device-specific validation is not worth it.
>
>Signed-off-by: Andrea Bolognani <abologna at redhat.com>
>---
> src/qemu/qemu_command.c | 181 ++++++++++++++++++----------------------
> 1 file changed, 81 insertions(+), 100 deletions(-)
>
>diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>index db7c3ad698..041e4bd7fb 100644
>--- a/src/qemu/qemu_command.c
>+++ b/src/qemu/qemu_command.c
>@@ -397,6 +397,59 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
>     return ret;
> }
>
>+
>+static int
>+qemuBuildVirtioDevStr(virBufferPtr buf,
>+                      const char *baseName,
>+                      virDomainDeviceAddressType type)
>+{
>+    const char *implName = NULL;
>+    int ret = -1;
>+
>+    switch (type) {
>+    case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
>+        implName = "pci";
>+        break;
>+
>+    case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO:
>+        implName = "device";
>+        break;
>+
>+    case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW:
>+        implName = "ccw";
>+        break;
>+
>+    case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390:
>+        implName = "s390";
>+        break;
>+
>+    case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE:
>+    case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL:
>+    case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID:
>+    case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB:
>+    case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO:
>+    case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA:
>+    case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM:
>+        virReportError(VIR_ERR_INTERNAL_ERROR,
>+                       _("Unexpected address type for '%s'"), baseName);
>+        goto done;

Just return -1;

>+
>+    case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE:
>+    case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST:
>+    default:
>+        virReportEnumRangeError(virDomainDeviceAddressType, type);
>+        goto done;

Here too.

It's unlikely we'll end up having to do something worth cleaning up
before we even validate the supplied parameters.

>+    }
>+
>+    virBufferAsprintf(buf, "%s-%s", baseName, implName);
>+

>+    ret = 0;
>+
>+ done:
>+    return ret;

Then this can be simply:

return 0;

Reviewed-by: Ján Tomko <jtomko at redhat.com>

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180906/ebe90ddc/attachment-0001.sig>