[libvirt] [PATCH 0/2] qemu: Unify generation of command line for virtio devices
-device virtio-blurb-pci Andrea Bolognani (2): tests: Fix use of virtio-serial for aarch64/virt qemu: Unify generation of command line for virtio devices src/qemu/qemu_command.c | 299 ++++++++++-------- .../mach-virt-console-virtio.args | 2 +- tests/qemuxml2argvtest.c | 3 +- .../mach-virt-console-virtio.xml | 4 +- tests/qemuxml2xmltest.c | 6 +- 5 files changed, 173 insertions(+), 141 deletions(-) -- 2.17.1
virtio-serial is an alias for virtio-serial-pci, which should not have been used for a PCIe-less aarch64/virt guest but it ended up being used anyway because the virtio-mmio capability was missing and the algorithm is buggy. Fix the test case so that we can fix the algorithm next. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemuxml2argvdata/mach-virt-console-virtio.args | 2 +- tests/qemuxml2argvtest.c | 3 ++- tests/qemuxml2xmloutdata/mach-virt-console-virtio.xml | 4 +++- tests/qemuxml2xmltest.c | 6 ++++-- 4 files changed, 10 insertions(+), 5 deletions(-) diff --git a/tests/qemuxml2argvdata/mach-virt-console-virtio.args b/tests/qemuxml2argvdata/mach-virt-console-virtio.args index d76c5892aa..f4a43ff7c6 100644 --- a/tests/qemuxml2argvdata/mach-virt-console-virtio.args +++ b/tests/qemuxml2argvdata/mach-virt-console-virtio.args @@ -20,6 +20,6 @@ server,nowait \ -rtc base=utc \ -no-shutdown \ -no-acpi \ --device virtio-serial,id=virtio-serial0 \ +-device virtio-serial-device,id=virtio-serial0 \ -chardev pty,id=charconsole0 \ -device virtconsole,chardev=charconsole0,id=console0 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 9bfe864597..144e595310 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1976,7 +1976,8 @@ mymain(void) QEMU_CAPS_DEVICE_USB_SERIAL); DO_TEST("mach-virt-console-native", QEMU_CAPS_DEVICE_PL011); - DO_TEST("mach-virt-console-virtio", NONE); + DO_TEST("mach-virt-console-virtio", + QEMU_CAPS_DEVICE_VIRTIO_MMIO); DO_TEST_PARSE_ERROR("mach-virt-serial-invalid-machine", NONE); DO_TEST("disk-ide-split", diff --git a/tests/qemuxml2xmloutdata/mach-virt-console-virtio.xml b/tests/qemuxml2xmloutdata/mach-virt-console-virtio.xml index 3e46cd2012..84e5c37ad9 100644 --- a/tests/qemuxml2xmloutdata/mach-virt-console-virtio.xml +++ b/tests/qemuxml2xmloutdata/mach-virt-console-virtio.xml @@ -18,7 +18,9 @@ <devices> <emulator>/usr/bin/qemu-system-aarch64</emulator> <controller type='usb' index='0' model='none'/> - <controller type='virtio-serial' index='0'/> + <controller type='virtio-serial' index='0'> + <address type='virtio-mmio'/> + </controller> <console type='pty'> <target type='virtio' port='0'/> </console> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 43fd4e5f0f..47da7a7201 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -644,8 +644,10 @@ mymain(void) QEMU_CAPS_DEVICE_PCIE_ROOT_PORT, QEMU_CAPS_DEVICE_QEMU_XHCI, QEMU_CAPS_DEVICE_USB_SERIAL); - DO_TEST("mach-virt-console-native", NONE); - DO_TEST("mach-virt-console-virtio", NONE); + DO_TEST("mach-virt-console-native", + QEMU_CAPS_DEVICE_PL011); + DO_TEST("mach-virt-console-virtio", + QEMU_CAPS_DEVICE_VIRTIO_MMIO); DO_TEST("balloon-device-auto", NONE); DO_TEST("balloon-device-period", NONE); -- 2.17.1
On Fri, Aug 31, 2018 at 04:03:09PM +0200, Andrea Bolognani wrote:
virtio-serial is an alias for virtio-serial-pci, which should not have been used for a PCIe-less aarch64/virt guest but it ended up being used anyway because the
I tried using the XML file with DO_TEST_CAPS_ARCH_LATEST and it resulted in a pcie-root-port being added. It seems all the capability files in our tests/qemucapabilities already support gpex-pcihost, so adding a test using contemporary capabilites might be worthwhile.
virtio-mmio capability was missing and the algorithm is buggy.
Fix the test case so that we can fix the algorithm next.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemuxml2argvdata/mach-virt-console-virtio.args | 2 +- tests/qemuxml2argvtest.c | 3 ++- tests/qemuxml2xmloutdata/mach-virt-console-virtio.xml | 4 +++- tests/qemuxml2xmltest.c | 6 ++++-- 4 files changed, 10 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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, so re-implementing such device-specific validation is not worth it. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 299 ++++++++++++++++++++++------------------ 1 file changed, 163 insertions(+), 136 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8aa20496bc..04554d958b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1812,6 +1812,62 @@ qemuBuildDriveDevCacheStr(virDomainDiskDefPtr disk, } +static char* +qemuBuildVirtioDevStr(const virDomainDeviceInfo *info, + const char *baseName) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *implName = NULL; + + switch ((virDomainDeviceAddressType)info->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 error; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST: + default: + virReportEnumRangeError(virDomainDeviceAddressType, + info->type); + goto error; + } + + virBufferAsprintf(&buf, "%s-%s", baseName, implName); + + if (virBufferCheckError(&buf) < 0) + goto error; + + return virBufferContentAndReset(&buf); + + error: + virBufferFreeAndReset(&buf); + return NULL; +} + + char * qemuBuildDiskDeviceStr(const virDomainDef *def, virDomainDiskDefPtr disk, @@ -1822,6 +1878,7 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, const char *bus = virDomainDiskQEMUBusTypeToString(disk->bus); const char *contAlias; char *backendAlias = NULL; + char *devStr = NULL; int controllerModel; if (qemuCheckDiskConfig(disk, qemuCaps) < 0) @@ -2002,21 +2059,19 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, break; case VIR_DOMAIN_DISK_BUS_VIRTIO: - if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { - virBufferAddLit(&opt, "virtio-blk-ccw"); - if (disk->iothread) - virBufferAsprintf(&opt, ",iothread=iothread%u", disk->iothread); - } else if (disk->info.type == - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) { - virBufferAddLit(&opt, "virtio-blk-s390"); - } else if (disk->info.type == - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) { - virBufferAddLit(&opt, "virtio-blk-device"); - } else { - virBufferAddLit(&opt, "virtio-blk-pci"); - if (disk->iothread) - virBufferAsprintf(&opt, ",iothread=iothread%u", disk->iothread); + + if (!(devStr = qemuBuildVirtioDevStr(&(disk->info), "virtio-blk"))) + goto error; + + virBufferAddStr(&opt, devStr); + VIR_FREE(devStr); + + if (disk->iothread && + (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW || + disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) { + virBufferAsprintf(&opt, ",iothread=iothread%u", disk->iothread); } + qemuBuildIoEventFdStr(&opt, disk->ioeventfd, qemuCaps); if (disk->event_idx && virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_EVENT_IDX)) { @@ -2150,6 +2205,7 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, return virBufferContentAndReset(&opt); error: + VIR_FREE(devStr); VIR_FREE(backendAlias); virBufferFreeAndReset(&opt); return NULL; @@ -2539,6 +2595,7 @@ qemuBuildFSDevStr(const virDomainDef *def, virQEMUCapsPtr qemuCaps) { virBuffer opt = VIR_BUFFER_INITIALIZER; + char *devStr = NULL; if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -2546,10 +2603,11 @@ qemuBuildFSDevStr(const virDomainDef *def, goto error; } - if (fs->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) - virBufferAddLit(&opt, "virtio-9p-ccw"); - else - virBufferAddLit(&opt, "virtio-9p-pci"); + if (!(devStr = qemuBuildVirtioDevStr(&(fs->info), "virtio-9p"))) + goto error; + + virBufferAddStr(&opt, devStr); + VIR_FREE(devStr); virBufferAsprintf(&opt, ",id=%s", fs->info.alias); virBufferAsprintf(&opt, ",fsdev=%s%s", @@ -2569,6 +2627,7 @@ qemuBuildFSDevStr(const virDomainDef *def, return virBufferContentAndReset(&opt); error: + VIR_FREE(devStr); virBufferFreeAndReset(&opt); return NULL; } @@ -2744,7 +2803,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, int *nusbcontroller) { virBuffer buf = VIR_BUFFER_INITIALIZER; - int address_type = def->info.type; + char *devStr = NULL; *devstr = NULL; @@ -2752,19 +2811,11 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: switch ((virDomainControllerModelSCSI) def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: - switch (address_type) { - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW: - virBufferAddLit(&buf, "virtio-scsi-ccw"); - break; - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390: - virBufferAddLit(&buf, "virtio-scsi-s390"); - break; - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO: - virBufferAddLit(&buf, "virtio-scsi-device"); - break; - default: - virBufferAddLit(&buf, "virtio-scsi-pci"); - } + if (!(devStr = qemuBuildVirtioDevStr(&(def->info), "virtio-scsi"))) + goto error; + + virBufferAddStr(&buf, devStr); + VIR_FREE(devStr); if (def->iothread) { virBufferAsprintf(&buf, ",iothread=iothread%u", @@ -2804,22 +2855,12 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, break; case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: - switch (address_type) { - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: - virBufferAddLit(&buf, "virtio-serial-pci"); - break; - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW: - virBufferAddLit(&buf, "virtio-serial-ccw"); - break; - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390: - virBufferAddLit(&buf, "virtio-serial-s390"); - break; - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO: - virBufferAddLit(&buf, "virtio-serial-device"); - break; - default: - virBufferAddLit(&buf, "virtio-serial"); - } + if (!(devStr = qemuBuildVirtioDevStr(&(def->info), "virtio-serial"))) + goto error; + + virBufferAddStr(&buf, devStr); + VIR_FREE(devStr); + virBufferAsprintf(&buf, ",id=%s", def->info.alias); if (def->opts.vioserial.ports != -1) { virBufferAsprintf(&buf, ",max_ports=%d", @@ -2948,6 +2989,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, return 0; error: + VIR_FREE(devStr); virBufferFreeAndReset(&buf); return -1; } @@ -3520,21 +3562,20 @@ qemuBuildNicDevStr(virDomainDefPtr def, const char *nic = net->model; bool usingVirtio = false; char macaddr[VIR_MAC_STRING_BUFLEN]; + char *devStr = NULL; if (STREQ(net->model, "virtio")) { - if (net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) - nic = "virtio-net-ccw"; - else if (net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) - nic = "virtio-net-s390"; - else if (net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) - nic = "virtio-net-device"; - else - nic = "virtio-net-pci"; - usingVirtio = true; + + if (!(devStr = qemuBuildVirtioDevStr(&(net->info), "virtio-net"))) + goto error; + + virBufferAddStr(&buf, devStr); + VIR_FREE(devStr); + } else { + virBufferAddStr(&buf, nic); } - virBufferAdd(&buf, nic, -1); if (usingVirtio && net->driver.virtio.txmode) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_TX_ALG)) { virBufferAddLit(&buf, ",tx="); @@ -3681,6 +3722,7 @@ qemuBuildNicDevStr(virDomainDefPtr def, return virBufferContentAndReset(&buf); error: + VIR_FREE(devStr); virBufferFreeAndReset(&buf); return NULL; } @@ -3914,6 +3956,7 @@ qemuBuildMemballoonCommandLine(virCommandPtr cmd, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; + char *devStr = NULL; if (STRPREFIX(def->os.machine, "s390-virtio") && virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390) && def->memballoon) @@ -3929,22 +3972,11 @@ qemuBuildMemballoonCommandLine(virCommandPtr cmd, return -1; } - switch (def->memballoon->info.type) { - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: - virBufferAddLit(&buf, "virtio-balloon-pci"); - break; - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW: - virBufferAddLit(&buf, "virtio-balloon-ccw"); - break; - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO: - virBufferAddLit(&buf, "virtio-balloon-device"); - break; - default: - virReportError(VIR_ERR_XML_ERROR, - _("memballoon unsupported with address type '%s'"), - virDomainDeviceAddressTypeToString(def->memballoon->info.type)); - goto error; - } + if (!(devStr = qemuBuildVirtioDevStr(&(def->memballoon->info), "virtio-balloon"))) + goto error; + + virBufferAddStr(&buf, devStr); + VIR_FREE(devStr); virBufferAsprintf(&buf, ",id=%s", def->memballoon->info.alias); if (qemuBuildDeviceAddressStr(&buf, def, &def->memballoon->info, qemuCaps) < 0) @@ -3969,6 +4001,7 @@ qemuBuildMemballoonCommandLine(virCommandPtr cmd, return 0; error: + VIR_FREE(devStr); virBufferFreeAndReset(&buf); return -1; } @@ -4040,63 +4073,57 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; - const char *suffix; - - if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - suffix = "-pci"; - } else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { - suffix = "-ccw"; - } else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) { - suffix = "-device"; - } else { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unsupported address type %s for virtio input device"), - virDomainDeviceAddressTypeToString(dev->info.type)); - goto error; - } + const char *baseName; + char *devStr; + int cap; + int ccwCap; switch ((virDomainInputType)dev->type) { case VIR_DOMAIN_INPUT_TYPE_MOUSE: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_MOUSE) || - (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW))) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("virtio-mouse is not supported by this QEMU binary")); - goto error; - } - virBufferAsprintf(&buf, "virtio-mouse%s,id=%s", suffix, dev->info.alias); + baseName = "virtio-mouse"; + cap = QEMU_CAPS_VIRTIO_MOUSE; + ccwCap = QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW; break; case VIR_DOMAIN_INPUT_TYPE_TABLET: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_TABLET) || - (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW))) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("virtio-tablet is not supported by this QEMU binary")); - goto error; - } - virBufferAsprintf(&buf, "virtio-tablet%s,id=%s", suffix, dev->info.alias); + baseName = "virtio-tablet"; + cap = QEMU_CAPS_VIRTIO_TABLET; + ccwCap = QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW; break; case VIR_DOMAIN_INPUT_TYPE_KBD: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_KEYBOARD) || - (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_KEYBOARD_CCW))) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("virtio-keyboard is not supported by this QEMU binary")); - goto error; - } - virBufferAsprintf(&buf, "virtio-keyboard%s,id=%s", suffix, dev->info.alias); + baseName = "virtio-keyboard"; + cap = QEMU_CAPS_VIRTIO_KEYBOARD; + ccwCap = QEMU_CAPS_DEVICE_VIRTIO_KEYBOARD_CCW; break; case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_INPUT_HOST)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("virtio-input-host is not supported by this QEMU binary")); - goto error; - } - virBufferAsprintf(&buf, "virtio-input-host%s,id=%s,evdev=", suffix, dev->info.alias); - virQEMUBuildBufferEscapeComma(&buf, dev->source.evdev); + baseName = "virtio-input-host"; + cap = QEMU_CAPS_VIRTIO_INPUT_HOST; + ccwCap = QEMU_CAPS_VIRTIO_INPUT_HOST; break; case VIR_DOMAIN_INPUT_TYPE_LAST: - break; + default: + virReportEnumRangeError(virDomainInputType, + dev->type); + goto error; + } + + if (!virQEMUCapsGet(qemuCaps, cap) || + (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && + !virQEMUCapsGet(qemuCaps, ccwCap))) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("%s is not supported by this QEMU binary"), + baseName); + goto error; + } + + if (!(devStr = qemuBuildVirtioDevStr(&(dev->info), baseName))) + goto error; + + virBufferAsprintf(&buf, "%s,id=%s", devStr, dev->info.alias); + VIR_FREE(devStr); + + if (dev->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) { + virBufferAddLit(&buf, ",evdev="); + virQEMUBuildBufferEscapeComma(&buf, dev->source.evdev); } if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) @@ -4111,6 +4138,7 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def, return virBufferContentAndReset(&buf); error: + VIR_FREE(devStr); virBufferFreeAndReset(&buf); return NULL; } @@ -4378,6 +4406,7 @@ qemuBuildDeviceVideoStr(const virDomainDef *def, { virBuffer buf = VIR_BUFFER_INITIALIZER; const char *model; + char *devStr = NULL; /* We try to chose the best model for primary video device by preferring * model with VGA compatibility mode. For some video devices on some @@ -4396,10 +4425,11 @@ qemuBuildDeviceVideoStr(const virDomainDef *def, } if (STREQ(model, "virtio-gpu")) { - if (video->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) - virBufferAsprintf(&buf, "%s-ccw", model); - else - virBufferAsprintf(&buf, "%s-pci", model); + if (!(devStr = qemuBuildVirtioDevStr(&(video->info), "virtio-gpu"))) + goto error; + + virBufferAddStr(&buf, devStr); + VIR_FREE(devStr); } else { virBufferAsprintf(&buf, "%s", model); } @@ -4462,6 +4492,7 @@ qemuBuildDeviceVideoStr(const virDomainDef *def, return virBufferContentAndReset(&buf); error: + VIR_FREE(devStr); virBufferFreeAndReset(&buf); return NULL; } @@ -5817,6 +5848,7 @@ qemuBuildRNGDevStr(const virDomainDef *def, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; + char *devStr = NULL; if (dev->model != VIR_DOMAIN_RNG_MODEL_VIRTIO || !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_RNG)) { @@ -5830,18 +5862,12 @@ qemuBuildRNGDevStr(const virDomainDef *def, dev->source.file)) goto error; - if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) - virBufferAsprintf(&buf, "virtio-rng-ccw,rng=obj%s,id=%s", - dev->info.alias, dev->info.alias); - else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) - virBufferAsprintf(&buf, "virtio-rng-s390,rng=obj%s,id=%s", - dev->info.alias, dev->info.alias); - else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) - virBufferAsprintf(&buf, "virtio-rng-device,rng=obj%s,id=%s", - dev->info.alias, dev->info.alias); - else - virBufferAsprintf(&buf, "virtio-rng-pci,rng=obj%s,id=%s", - dev->info.alias, dev->info.alias); + if (!(devStr = qemuBuildVirtioDevStr(&(dev->info), "virtio-rng"))) + goto error; + + virBufferAsprintf(&buf, "%s,rng=obj%s,id=%s", + devStr, dev->info.alias, dev->info.alias); + VIR_FREE(devStr); if (dev->rate > 0) { virBufferAsprintf(&buf, ",max-bytes=%u", dev->rate); @@ -5862,6 +5888,7 @@ qemuBuildRNGDevStr(const virDomainDef *def, return virBufferContentAndReset(&buf); error: + VIR_FREE(devStr); virBufferFreeAndReset(&buf); return NULL; } -- 2.17.1
On Fri, Aug 31, 2018 at 04:03:10PM +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, so re-implementing such device-specific validation is not worth it.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 299 ++++++++++++++++++++++------------------ 1 file changed, 163 insertions(+), 136 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8aa20496bc..04554d958b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1812,6 +1812,62 @@ qemuBuildDriveDevCacheStr(virDomainDiskDefPtr disk, }
+static char* +qemuBuildVirtioDevStr(const virDomainDeviceInfo *info, + const char *baseName) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *implName = NULL; + + switch ((virDomainDeviceAddressType)info->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);
'%s'
+ goto error; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST: + default: + virReportEnumRangeError(virDomainDeviceAddressType, + info->type); + goto error; + } + + virBufferAsprintf(&buf, "%s-%s", baseName, implName); +
buf is used exactly once in this function, could have been just virAsprintf. Or, even better, since all the calls are followed by adding the string to a buffer, just pass the buffer as the function argument.
+ if (virBufferCheckError(&buf) < 0) + goto error; + + return virBufferContentAndReset(&buf); + + error: + virBufferFreeAndReset(&buf); + return NULL; +} + + char * qemuBuildDiskDeviceStr(const virDomainDef *def, virDomainDiskDefPtr disk, @@ -1822,6 +1878,7 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, const char *bus = virDomainDiskQEMUBusTypeToString(disk->bus); const char *contAlias; char *backendAlias = NULL; + char *devStr = NULL; int controllerModel;
if (qemuCheckDiskConfig(disk, qemuCaps) < 0) @@ -2002,21 +2059,19 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, break;
case VIR_DOMAIN_DISK_BUS_VIRTIO: - if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { - virBufferAddLit(&opt, "virtio-blk-ccw"); - if (disk->iothread) - virBufferAsprintf(&opt, ",iothread=iothread%u", disk->iothread); - } else if (disk->info.type == - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) { - virBufferAddLit(&opt, "virtio-blk-s390"); - } else if (disk->info.type == - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) { - virBufferAddLit(&opt, "virtio-blk-device"); - } else { - virBufferAddLit(&opt, "virtio-blk-pci"); - if (disk->iothread) - virBufferAsprintf(&opt, ",iothread=iothread%u", disk->iothread); + + if (!(devStr = qemuBuildVirtioDevStr(&(disk->info), "virtio-blk")))
&disk->info is enough. Or even simpler: disk->info.type
+ goto error; + + virBufferAddStr(&opt, devStr); + VIR_FREE(devStr); + + if (disk->iothread && + (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW || + disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) { + virBufferAsprintf(&opt, ",iothread=iothread%u", disk->iothread);
The iothread change could be separated for an easier-to-read patch. Also, formatting it unconditionally would be nicer - do we even need to check for the address type? We format it unconditionally for the virtio-scsi controller.
} + qemuBuildIoEventFdStr(&opt, disk->ioeventfd, qemuCaps); if (disk->event_idx && virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_EVENT_IDX)) { @@ -3520,21 +3562,20 @@ qemuBuildNicDevStr(virDomainDefPtr def, const char *nic = net->model; bool usingVirtio = false; char macaddr[VIR_MAC_STRING_BUFLEN]; + char *devStr = NULL;
if (STREQ(net->model, "virtio")) { - if (net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) - nic = "virtio-net-ccw"; - else if (net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) - nic = "virtio-net-s390"; - else if (net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) - nic = "virtio-net-device"; - else - nic = "virtio-net-pci"; - usingVirtio = true; + + if (!(devStr = qemuBuildVirtioDevStr(&(net->info), "virtio-net"))) + goto error; + + virBufferAddStr(&buf, devStr); + VIR_FREE(devStr); + } else { + virBufferAddStr(&buf, nic);
virBufferAddStr(&buf, net->model); Since we no longer use 'nic' unconditionally.
}
- virBufferAdd(&buf, nic, -1); if (usingVirtio && net->driver.virtio.txmode) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_TX_ALG)) { virBufferAddLit(&buf, ",tx="); @@ -3681,6 +3722,7 @@ qemuBuildNicDevStr(virDomainDefPtr def, return virBufferContentAndReset(&buf);
error: + VIR_FREE(devStr); virBufferFreeAndReset(&buf); return NULL; } @@ -3914,6 +3956,7 @@ qemuBuildMemballoonCommandLine(virCommandPtr cmd, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; + char *devStr = NULL;
if (STRPREFIX(def->os.machine, "s390-virtio") && virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390) && def->memballoon) @@ -3929,22 +3972,11 @@ qemuBuildMemballoonCommandLine(virCommandPtr cmd, return -1; }
- switch (def->memballoon->info.type) { - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: - virBufferAddLit(&buf, "virtio-balloon-pci"); - break; - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW: - virBufferAddLit(&buf, "virtio-balloon-ccw"); - break; - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO: - virBufferAddLit(&buf, "virtio-balloon-device"); - break; - default: - virReportError(VIR_ERR_XML_ERROR, - _("memballoon unsupported with address type '%s'"), - virDomainDeviceAddressTypeToString(def->memballoon->info.type)); - goto error; - } + if (!(devStr = qemuBuildVirtioDevStr(&(def->memballoon->info), "virtio-balloon"))) + goto error; + + virBufferAddStr(&buf, devStr); + VIR_FREE(devStr);
virBufferAsprintf(&buf, ",id=%s", def->memballoon->info.alias); if (qemuBuildDeviceAddressStr(&buf, def, &def->memballoon->info, qemuCaps) < 0) @@ -3969,6 +4001,7 @@ qemuBuildMemballoonCommandLine(virCommandPtr cmd, return 0;
error: + VIR_FREE(devStr); virBufferFreeAndReset(&buf); return -1; } @@ -4040,63 +4073,57 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; - const char *suffix; - - if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - suffix = "-pci"; - } else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { - suffix = "-ccw"; - } else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) { - suffix = "-device"; - } else { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unsupported address type %s for virtio input device"), - virDomainDeviceAddressTypeToString(dev->info.type)); - goto error; - } + const char *baseName; + char *devStr; + int cap; + int ccwCap;
switch ((virDomainInputType)dev->type) { case VIR_DOMAIN_INPUT_TYPE_MOUSE: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_MOUSE) || - (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW))) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("virtio-mouse is not supported by this QEMU binary")); - goto error; - } - virBufferAsprintf(&buf, "virtio-mouse%s,id=%s", suffix, dev->info.alias); + baseName = "virtio-mouse"; + cap = QEMU_CAPS_VIRTIO_MOUSE; + ccwCap = QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW; break; case VIR_DOMAIN_INPUT_TYPE_TABLET: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_TABLET) || - (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW))) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("virtio-tablet is not supported by this QEMU binary")); - goto error; - }
The capability check refactor is out of scope of this patch. Also, they should be in the *Validate functions, not the command line formatter. (Sadly, we need a separate capability set for CCW devices, because the support was added later - as evidenced by: git grep virtio-mouse tests/qemucapabilitiesdata/*s390* #)
- virBufferAsprintf(&buf, "virtio-tablet%s,id=%s", suffix, dev->info.alias); + baseName = "virtio-tablet"; + cap = QEMU_CAPS_VIRTIO_TABLET; + ccwCap = QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW; break; case VIR_DOMAIN_INPUT_TYPE_KBD: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_KEYBOARD) || - (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_KEYBOARD_CCW))) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("virtio-keyboard is not supported by this QEMU binary")); - goto error; - } - virBufferAsprintf(&buf, "virtio-keyboard%s,id=%s", suffix, dev->info.alias); + baseName = "virtio-keyboard"; + cap = QEMU_CAPS_VIRTIO_KEYBOARD; + ccwCap = QEMU_CAPS_DEVICE_VIRTIO_KEYBOARD_CCW; break; case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_INPUT_HOST)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("virtio-input-host is not supported by this QEMU binary")); - goto error; - } - virBufferAsprintf(&buf, "virtio-input-host%s,id=%s,evdev=", suffix, dev->info.alias); - virQEMUBuildBufferEscapeComma(&buf, dev->source.evdev); + baseName = "virtio-input-host"; + cap = QEMU_CAPS_VIRTIO_INPUT_HOST; + ccwCap = QEMU_CAPS_VIRTIO_INPUT_HOST; break; case VIR_DOMAIN_INPUT_TYPE_LAST: - break; + default: + virReportEnumRangeError(virDomainInputType, + dev->type); + goto error; + }
Unrelated virReportEnumRangeError addition.
+ + if (!virQEMUCapsGet(qemuCaps, cap) || + (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && + !virQEMUCapsGet(qemuCaps, ccwCap))) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("%s is not supported by this QEMU binary"), + baseName); + goto error; + } + + if (!(devStr = qemuBuildVirtioDevStr(&(dev->info), baseName))) + goto error; + + virBufferAsprintf(&buf, "%s,id=%s", devStr, dev->info.alias); + VIR_FREE(devStr); + + if (dev->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) { + virBufferAddLit(&buf, ",evdev="); + virQEMUBuildBufferEscapeComma(&buf, dev->source.evdev);
Personally, I'd also separate all the changes that remove the additional attributes from the device name.
}
if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) @@ -4111,6 +4138,7 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def, return virBufferContentAndReset(&buf);
error: + VIR_FREE(devStr); virBufferFreeAndReset(&buf); return NULL; } @@ -4378,6 +4406,7 @@ qemuBuildDeviceVideoStr(const virDomainDef *def, { virBuffer buf = VIR_BUFFER_INITIALIZER; const char *model; + char *devStr = NULL;
/* We try to chose the best model for primary video device by preferring * model with VGA compatibility mode. For some video devices on some @@ -4396,10 +4425,11 @@ qemuBuildDeviceVideoStr(const virDomainDef *def, }
if (STREQ(model, "virtio-gpu")) { - if (video->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) - virBufferAsprintf(&buf, "%s-ccw", model); - else - virBufferAsprintf(&buf, "%s-pci", model); + if (!(devStr = qemuBuildVirtioDevStr(&(video->info), "virtio-gpu"))) + goto error; + + virBufferAddStr(&buf, devStr); + VIR_FREE(devStr); } else { virBufferAsprintf(&buf, "%s", model); } @@ -4462,6 +4492,7 @@ qemuBuildDeviceVideoStr(const virDomainDef *def, return virBufferContentAndReset(&buf);
error: + VIR_FREE(devStr); virBufferFreeAndReset(&buf); return NULL; } @@ -5817,6 +5848,7 @@ qemuBuildRNGDevStr(const virDomainDef *def, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; + char *devStr = NULL;
if (dev->model != VIR_DOMAIN_RNG_MODEL_VIRTIO || !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_RNG)) { @@ -5830,18 +5862,12 @@ qemuBuildRNGDevStr(const virDomainDef *def, dev->source.file)) goto error;
- if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) - virBufferAsprintf(&buf, "virtio-rng-ccw,rng=obj%s,id=%s", - dev->info.alias, dev->info.alias); - else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) - virBufferAsprintf(&buf, "virtio-rng-s390,rng=obj%s,id=%s", - dev->info.alias, dev->info.alias); - else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) - virBufferAsprintf(&buf, "virtio-rng-device,rng=obj%s,id=%s", - dev->info.alias, dev->info.alias);
Oh, fun. Not only we merged this copy & paste rng= attribute addition, it was later amended to add the id= attribute. Then broken into separate lines by a separate commit.
- else - virBufferAsprintf(&buf, "virtio-rng-pci,rng=obj%s,id=%s", - dev->info.alias, dev->info.alias); + if (!(devStr = qemuBuildVirtioDevStr(&(dev->info), "virtio-rng"))) + goto error; + + virBufferAsprintf(&buf, "%s,rng=obj%s,id=%s", + devStr, dev->info.alias, dev->info.alias); + VIR_FREE(devStr);
if (dev->rate > 0) { virBufferAsprintf(&buf, ",max-bytes=%u", dev->rate);
Jano
On Tue, 2018-09-04 at 16:09 +0200, Ján Tomko wrote:
On Fri, Aug 31, 2018 at 04:03:10PM +0200, Andrea Bolognani wrote:
+static char* +qemuBuildVirtioDevStr(const virDomainDeviceInfo *info, + const char *baseName) [...] + virBufferAsprintf(&buf, "%s-%s", baseName, implName);
buf is used exactly once in this function, could have been just virAsprintf.
Or, even better, since all the calls are followed by adding the string to a buffer, just pass the buffer as the function argument.
I did it that way initially, but then I changed it to return a char* to be consistent with other qemuBuild*DevStr(). I can definitely change it back, but perhaps a different name would be more appropriate at that point. [...]
+ if (!(devStr = qemuBuildVirtioDevStr(&(disk->info), "virtio-blk")))
&disk->info is enough.
Or even simpler: disk->info.type
Okay, I'll go with the latter. [...]
+ if (disk->iothread && + (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW || + disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) { + virBufferAsprintf(&opt, ",iothread=iothread%u", disk->iothread);
The iothread change could be separated for an easier-to-read patch.
Agreed, it should have been that way to begin with.
Also, formatting it unconditionally would be nicer - do we even need to check for the address type? We format it unconditionally for the virtio-scsi controller.
Not sure. I don't really know anything about iothreads, so I purposefully steered clear of changing that part :) [...]
+ virBufferAddStr(&buf, devStr); + VIR_FREE(devStr); + } else { + virBufferAddStr(&buf, nic);
virBufferAddStr(&buf, net->model);
Since we no longer use 'nic' unconditionally.
Okay. [...]
case VIR_DOMAIN_INPUT_TYPE_TABLET: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_TABLET) || - (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW))) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("virtio-tablet is not supported by this QEMU binary")); - goto error; - }
The capability check refactor is out of scope of this patch. Also, they should be in the *Validate functions, not the command line formatter.
Agreed, I'll split that part off. [...]
case VIR_DOMAIN_INPUT_TYPE_LAST: - break; + default: + virReportEnumRangeError(virDomainInputType, + dev->type); + goto error; + }
Unrelated virReportEnumRangeError addition.
I'll make that a separate patch too. [...]
+ if (dev->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) { + virBufferAddLit(&buf, ",evdev="); + virQEMUBuildBufferEscapeComma(&buf, dev->source.evdev);
Personally, I'd also separate all the changes that remove the additional attributes from the device name.
Agreed. [...]
- if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) - virBufferAsprintf(&buf, "virtio-rng-ccw,rng=obj%s,id=%s", - dev->info.alias, dev->info.alias); - else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) - virBufferAsprintf(&buf, "virtio-rng-s390,rng=obj%s,id=%s", - dev->info.alias, dev->info.alias); - else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) - virBufferAsprintf(&buf, "virtio-rng-device,rng=obj%s,id=%s", - dev->info.alias, dev->info.alias);
Oh, fun. Not only we merged this copy & paste rng= attribute addition, it was later amended to add the id= attribute. Then broken into separate lines by a separate commit.
Well, at least now we're going to get rid of the duplication :) -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Sep 04, 2018 at 05:32:51PM +0200, Andrea Bolognani wrote:
On Tue, 2018-09-04 at 16:09 +0200, Ján Tomko wrote:
On Fri, Aug 31, 2018 at 04:03:10PM +0200, Andrea Bolognani wrote:
+static char* +qemuBuildVirtioDevStr(const virDomainDeviceInfo *info, + const char *baseName) [...] + virBufferAsprintf(&buf, "%s-%s", baseName, implName);
buf is used exactly once in this function, could have been just virAsprintf.
Or, even better, since all the calls are followed by adding the string to a buffer, just pass the buffer as the function argument.
I did it that way initially, but then I changed it to return a char* to be consistent with other qemuBuild*DevStr(). I can definitely change it back, but perhaps a different name would be more appropriate at that point.
OTOH, many qemuBuild.*Str which only build a repetitive part of the string have a virBuffer as the first argument. If the DevStr inconsistency bothers you, maybe 'qemuBuildVirtioDeviceStr' or qemuBuildVirtioDeviceSuffix{,Str}? Jano
participants (2)
-
Andrea Bolognani -
Ján Tomko