[libvirt] [PATCH 00/11] qemu: format CCID controllers after USB hubs
https://bugzilla.redhat.com/show_bug.cgi?id=1599971 Ján Tomko (11): qemu: move out legacy USB controller formatting qemu: exit early if USB_CONTROLLER_MODEL_NONE is present Add qemuBuildDomainForbidLegacyUSBController qemu: separate counting of legacy USB controllers qemu: separate counting of USB controllers qemuBuildControllerDevStr: remove nusbcontroller argument qemu: Introduce qemuBuildControllersByTypeCommandLine qemu: format CCID controllers after USB hubs qemuBuildControllersByTypeCommandLine: free devstr in the cleanup section rename qemuBuildControllerDevCommandLine qemuBuildControllersCommandLine: use i instead of j as the counter src/qemu/qemu_command.c | 222 +++++++++++------- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_hotplug.c | 2 +- tests/qemuxml2argvdata/chardev-reconnect.args | 2 +- tests/qemuxml2argvdata/controller-order.args | 2 +- tests/qemuxml2argvdata/name-escape.args | 2 +- .../smartcard-controller.args | 2 +- .../smartcard-host-certificates-database.args | 2 +- .../smartcard-host-certificates.args | 2 +- tests/qemuxml2argvdata/smartcard-host.args | 2 +- .../smartcard-passthrough-spicevmc.args | 2 +- .../smartcard-passthrough-tcp.args | 2 +- tests/qemuxml2argvdata/user-aliases.args | 2 +- 13 files changed, 150 insertions(+), 97 deletions(-) -- 2.20.1
Move out the code formatting "-usb" on the QEMU command line. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_command.c | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 822d5f8669..d6b7d11cc6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3021,6 +3021,26 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, } +static int +qemuBuildLegacyUSBControllerCommandLine(virCommandPtr cmd, + const virDomainDef *def, + int usbcontroller) +{ + if (usbcontroller == 0 && + !qemuDomainIsQ35(def) && + !qemuDomainIsARMVirt(def) && + !qemuDomainIsRISCVVirt(def) && + !ARCH_IS_S390(def->os.arch)) { + /* We haven't added any USB controller yet, but we haven't been asked + * not to add one either. Add a legacy USB controller, unless we're + * creating a kind of guest we want to keep legacy-free */ + virCommandAddArg(cmd, "-usb"); + } + + return 0; +} + + /** * qemuBuildSkipController: * @controller: Controller to check @@ -3156,16 +3176,8 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, } } - if (usbcontroller == 0 && - !qemuDomainIsQ35(def) && - !qemuDomainIsARMVirt(def) && - !qemuDomainIsRISCVVirt(def) && - !ARCH_IS_S390(def->os.arch)) { - /* We haven't added any USB controller yet, but we haven't been asked - * not to add one either. Add a legacy USB controller, unless we're - * creating a kind of guest we want to keep legacy-free */ - virCommandAddArg(cmd, "-usb"); - } + if (qemuBuildLegacyUSBControllerCommandLine(cmd, def, usbcontroller) < 0) + goto cleanup; ret = 0; -- 2.20.1
This removes the need to mark it in the 'usbcontroller' variable. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_command.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d6b7d11cc6..80b05efa03 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3026,6 +3026,19 @@ qemuBuildLegacyUSBControllerCommandLine(virCommandPtr cmd, const virDomainDef *def, int usbcontroller) { + size_t i; + + for (i = 0; i < def->ncontrollers; i++) { + virDomainControllerDefPtr cont = def->controllers[i]; + + if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_USB) + continue; + + /* If we have mode='none', there are no other USB controllers */ + if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) + return 0; + } + if (usbcontroller == 0 && !qemuDomainIsQ35(def) && !qemuDomainIsARMVirt(def) && @@ -3127,7 +3140,6 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, /* skip USB controllers with type none.*/ if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) { - usbcontroller = -1; /* mark we don't want a controller */ continue; } -- 2.20.1
Shorten some long conditions. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_command.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 80b05efa03..cce5520783 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3021,6 +3021,18 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, } +static bool +qemuBuildDomainForbidLegacyUSBController(const virDomainDef *def) +{ + if (qemuDomainIsQ35(def) || + qemuDomainIsARMVirt(def) || + qemuDomainIsRISCVVirt(def)) + return true; + + return false; +} + + static int qemuBuildLegacyUSBControllerCommandLine(virCommandPtr cmd, const virDomainDef *def, @@ -3040,9 +3052,7 @@ qemuBuildLegacyUSBControllerCommandLine(virCommandPtr cmd, } if (usbcontroller == 0 && - !qemuDomainIsQ35(def) && - !qemuDomainIsARMVirt(def) && - !qemuDomainIsRISCVVirt(def) && + !qemuBuildDomainForbidLegacyUSBController(def) && !ARCH_IS_S390(def->os.arch)) { /* We haven't added any USB controller yet, but we haven't been asked * not to add one either. Add a legacy USB controller, unless we're @@ -3145,9 +3155,7 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT && - !qemuDomainIsQ35(def) && - !qemuDomainIsARMVirt(def) && - !qemuDomainIsRISCVVirt(def)) { + !qemuBuildDomainForbidLegacyUSBController(def)) { /* An appropriate default USB controller model should already * have been selected in qemuDomainDeviceDefPostParse(); if -- 2.20.1
On 01/16/2019 02:56 AM, Ján Tomko wrote:
Shorten some long conditions.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_command.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 80b05efa03..cce5520783 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3021,6 +3021,18 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, }
+static bool +qemuBuildDomainForbidLegacyUSBController(const virDomainDef *def) +{ + if (qemuDomainIsQ35(def) || + qemuDomainIsARMVirt(def) || + qemuDomainIsRISCVVirt(def)) + return true; + + return false; +} + + static int qemuBuildLegacyUSBControllerCommandLine(virCommandPtr cmd, const virDomainDef *def, @@ -3040,9 +3052,7 @@ qemuBuildLegacyUSBControllerCommandLine(virCommandPtr cmd, }
if (usbcontroller == 0 && - !qemuDomainIsQ35(def) && - !qemuDomainIsARMVirt(def) && - !qemuDomainIsRISCVVirt(def) && + !qemuBuildDomainForbidLegacyUSBController(def) && !ARCH_IS_S390(def->os.arch)) {
From the patch context it looks like the S390 condition was missed here. I see the comment below explains it but this confused me for a minute
- Cole
Count them in qemuBuildLegacyUSBControllerCommandLine to remove yet another variable accessed from the loop in qemuBuildControllerDevCommandLine. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_command.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index cce5520783..014bd031a8 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3039,6 +3039,7 @@ qemuBuildLegacyUSBControllerCommandLine(virCommandPtr cmd, int usbcontroller) { size_t i; + size_t nlegacy = 0; for (i = 0; i < def->ncontrollers; i++) { virDomainControllerDefPtr cont = def->controllers[i]; @@ -3049,6 +3050,16 @@ qemuBuildLegacyUSBControllerCommandLine(virCommandPtr cmd, /* If we have mode='none', there are no other USB controllers */ if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) return 0; + + if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT) + nlegacy++; + } + + if (nlegacy > 1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Multiple legacy USB controllers are " + "not supported")); + return -1; } if (usbcontroller == 0 && @@ -3109,7 +3120,6 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, { size_t i, j; int usbcontroller = 0; - bool usblegacy = false; int contOrder[] = { /* * List of controller types that we add commandline args for, @@ -3169,13 +3179,6 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, * (see 548ba43028 for the full story), so we skip * qemuBuildControllerDevStr() but we don't ultimately end * up adding the legacy USB controller */ - if (usblegacy) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Multiple legacy USB controllers are " - "not supported")); - goto cleanup; - } - usblegacy = true; continue; } -- 2.20.1
qemuBuildLegacyUSBControllerCommandLine is the only place where we need to count the USB controllers. Count them again instead of keeping track in a variable passed to qemuBuildControllerDevStr. This removes the need for another variable in the loop in qemuBuildControllerDevCommandLine. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_command.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 014bd031a8..22f041a956 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3035,11 +3035,11 @@ qemuBuildDomainForbidLegacyUSBController(const virDomainDef *def) static int qemuBuildLegacyUSBControllerCommandLine(virCommandPtr cmd, - const virDomainDef *def, - int usbcontroller) + const virDomainDef *def) { size_t i; size_t nlegacy = 0; + size_t nusb = 0; for (i = 0; i < def->ncontrollers; i++) { virDomainControllerDefPtr cont = def->controllers[i]; @@ -3053,6 +3053,8 @@ qemuBuildLegacyUSBControllerCommandLine(virCommandPtr cmd, if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT) nlegacy++; + else + nusb++; } if (nlegacy > 1) { @@ -3062,7 +3064,7 @@ qemuBuildLegacyUSBControllerCommandLine(virCommandPtr cmd, return -1; } - if (usbcontroller == 0 && + if (nusb == 0 && !qemuBuildDomainForbidLegacyUSBController(def) && !ARCH_IS_S390(def->os.arch)) { /* We haven't added any USB controller yet, but we haven't been asked @@ -3199,7 +3201,7 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, } } - if (qemuBuildLegacyUSBControllerCommandLine(cmd, def, usbcontroller) < 0) + if (qemuBuildLegacyUSBControllerCommandLine(cmd, def) < 0) goto cleanup; ret = 0; -- 2.20.1
Now that it's no longer needed, remove the argument. This removes the last helper variable in qemuBuildControllerDevCommandLine. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_command.c | 10 ++-------- src/qemu/qemu_command.h | 3 +-- src/qemu/qemu_hotplug.c | 2 +- 3 files changed, 4 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 22f041a956..f95231666f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2833,8 +2833,7 @@ int qemuBuildControllerDevStr(const virDomainDef *domainDef, virDomainControllerDefPtr def, virQEMUCapsPtr qemuCaps, - char **devstr, - int *nusbcontroller) + char **devstr) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -2913,9 +2912,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, if (qemuBuildUSBControllerDevStr(domainDef, def, qemuCaps, &buf) == -1) goto error; - if (nusbcontroller) - *nusbcontroller += 1; - break; case VIR_DOMAIN_CONTROLLER_TYPE_PCI: { @@ -3121,7 +3117,6 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, virQEMUCapsPtr qemuCaps) { size_t i, j; - int usbcontroller = 0; int contOrder[] = { /* * List of controller types that we add commandline args for, @@ -3184,8 +3179,7 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, continue; } - if (qemuBuildControllerDevStr(def, cont, qemuCaps, - &devstr, &usbcontroller) < 0) + if (qemuBuildControllerDevStr(def, cont, qemuCaps, &devstr) < 0) goto cleanup; if (devstr) { diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 1a4f1f5da4..d9f8a818b5 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -117,8 +117,7 @@ char int qemuBuildControllerDevStr(const virDomainDef *domainDef, virDomainControllerDefPtr def, virQEMUCapsPtr qemuCaps, - char **devstr, - int *nusbcontroller); + char **devstr); int qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, const char *alias, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a1c3ca999b..c3d935a412 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1022,7 +1022,7 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, if (qemuAssignDeviceControllerAlias(vm->def, priv->qemuCaps, controller) < 0) goto cleanup; - if (qemuBuildControllerDevStr(vm->def, controller, priv->qemuCaps, &devstr, NULL) < 0) + if (qemuBuildControllerDevStr(vm->def, controller, priv->qemuCaps, &devstr) < 0) goto cleanup; if (!devstr) -- 2.20.1
Now that the inner loop does not require any other variables, it can be easily separated. Apart from reducing the indentation level this will allow it to be called from different code paths. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_command.c | 118 +++++++++++++++++++++++----------------- 1 file changed, 68 insertions(+), 50 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f95231666f..91c7ec5f7b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3111,12 +3111,77 @@ qemuBuildSkipController(const virDomainControllerDef *controller, } +static int +qemuBuildControllersByTypeCommandLine(virCommandPtr cmd, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps, + virDomainControllerType type) +{ + int ret = -1; + size_t i; + + for (i = 0; i < def->ncontrollers; i++) { + virDomainControllerDefPtr cont = def->controllers[i]; + char *devstr; + + if (cont->type != type) + continue; + + if (qemuBuildSkipController(cont, def)) + continue; + + /* skip USB controllers with type none.*/ + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) { + continue; + } + + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT && + !qemuBuildDomainForbidLegacyUSBController(def)) { + + /* An appropriate default USB controller model should already + * have been selected in qemuDomainDeviceDefPostParse(); if + * we still have no model by now, we have to fall back to the + * legacy USB controller. + * + * Note that we *don't* want to end up with the legacy USB + * controller for q35 and virt machines, so we go ahead and + * fail in qemuBuildControllerDevStr(); on the other hand, + * for s390 machines we want to ignore any USB controller + * (see 548ba43028 for the full story), so we skip + * qemuBuildControllerDevStr() but we don't ultimately end + * up adding the legacy USB controller */ + continue; + } + + if (qemuBuildControllerDevStr(def, cont, qemuCaps, &devstr) < 0) + goto cleanup; + + if (devstr) { + if (qemuCommandAddExtDevice(cmd, &cont->info) < 0) { + VIR_FREE(devstr); + goto cleanup; + } + + virCommandAddArg(cmd, "-device"); + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); + } + } + + ret = 0; + cleanup: + return ret; +} + + static int qemuBuildControllerDevCommandLine(virCommandPtr cmd, const virDomainDef *def, virQEMUCapsPtr qemuCaps) { - size_t i, j; + size_t j; int contOrder[] = { /* * List of controller types that we add commandline args for, @@ -3144,55 +3209,8 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, int ret = -1; for (j = 0; j < ARRAY_CARDINALITY(contOrder); j++) { - for (i = 0; i < def->ncontrollers; i++) { - virDomainControllerDefPtr cont = def->controllers[i]; - char *devstr; - - if (cont->type != contOrder[j]) - continue; - - if (qemuBuildSkipController(cont, def)) - continue; - - /* skip USB controllers with type none.*/ - if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && - cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) { - continue; - } - - if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && - cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT && - !qemuBuildDomainForbidLegacyUSBController(def)) { - - /* An appropriate default USB controller model should already - * have been selected in qemuDomainDeviceDefPostParse(); if - * we still have no model by now, we have to fall back to the - * legacy USB controller. - * - * Note that we *don't* want to end up with the legacy USB - * controller for q35 and virt machines, so we go ahead and - * fail in qemuBuildControllerDevStr(); on the other hand, - * for s390 machines we want to ignore any USB controller - * (see 548ba43028 for the full story), so we skip - * qemuBuildControllerDevStr() but we don't ultimately end - * up adding the legacy USB controller */ - continue; - } - - if (qemuBuildControllerDevStr(def, cont, qemuCaps, &devstr) < 0) - goto cleanup; - - if (devstr) { - if (qemuCommandAddExtDevice(cmd, &cont->info) < 0) { - VIR_FREE(devstr); - goto cleanup; - } - - virCommandAddArg(cmd, "-device"); - virCommandAddArg(cmd, devstr); - VIR_FREE(devstr); - } - } + if (qemuBuildControllersByTypeCommandLine(cmd, def, qemuCaps, contOrder[j]) < 0) + goto cleanup; } if (qemuBuildLegacyUSBControllerCommandLine(cmd, def) < 0) -- 2.20.1
Since they go on the USB bus, format them after USB hubs. https://bugzilla.redhat.com/show_bug.cgi?id=1375402 Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_command.c | 8 +++++++- tests/qemuxml2argvdata/chardev-reconnect.args | 2 +- tests/qemuxml2argvdata/controller-order.args | 2 +- tests/qemuxml2argvdata/name-escape.args | 2 +- tests/qemuxml2argvdata/smartcard-controller.args | 2 +- .../smartcard-host-certificates-database.args | 2 +- tests/qemuxml2argvdata/smartcard-host-certificates.args | 2 +- tests/qemuxml2argvdata/smartcard-host.args | 2 +- .../qemuxml2argvdata/smartcard-passthrough-spicevmc.args | 2 +- tests/qemuxml2argvdata/smartcard-passthrough-tcp.args | 2 +- tests/qemuxml2argvdata/user-aliases.args | 2 +- 11 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 91c7ec5f7b..7aafe235e2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3197,6 +3197,9 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, * one. Likewise, we don't do anything for the primary IDE * controller on an i440fx machine or primary SATA on q35, but * we do add those beyond these two exceptions. + * + * CCID controllers are formated separately after USB hubs, + * because they go on the USB bus. */ VIR_DOMAIN_CONTROLLER_TYPE_PCI, VIR_DOMAIN_CONTROLLER_TYPE_USB, @@ -3204,7 +3207,6 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, VIR_DOMAIN_CONTROLLER_TYPE_IDE, VIR_DOMAIN_CONTROLLER_TYPE_SATA, VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL, - VIR_DOMAIN_CONTROLLER_TYPE_CCID, }; int ret = -1; @@ -10602,6 +10604,10 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildHubCommandLine(cmd, def, qemuCaps) < 0) goto error; + if (qemuBuildControllersByTypeCommandLine(cmd, def, qemuCaps, + VIR_DOMAIN_CONTROLLER_TYPE_CCID) < 0) + goto error; + if (qemuBuildDisksCommandLine(cmd, def, qemuCaps) < 0) goto error; diff --git a/tests/qemuxml2argvdata/chardev-reconnect.args b/tests/qemuxml2argvdata/chardev-reconnect.args index e88e083291..baff469c7b 100644 --- a/tests/qemuxml2argvdata/chardev-reconnect.args +++ b/tests/qemuxml2argvdata/chardev-reconnect.args @@ -22,8 +22,8 @@ server,nowait \ -no-acpi \ -device virtio-serial-pci,id=virtio-serial1,bus=pci.0,addr=0xa \ -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x3 \ --device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -usb \ +-device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -chardev socket,id=charsmartcard0,path=/tmp/channel/asdf,reconnect=20 \ -device ccid-card-passthru,chardev=charsmartcard0,id=smartcard0,bus=ccid0.0 \ -chardev socket,id=charchannel0,host=localhost,port=1234,reconnect=10 \ diff --git a/tests/qemuxml2argvdata/controller-order.args b/tests/qemuxml2argvdata/controller-order.args index 2477cd2e1c..6bb4942223 100644 --- a/tests/qemuxml2argvdata/controller-order.args +++ b/tests/qemuxml2argvdata/controller-order.args @@ -21,8 +21,8 @@ nowait \ -boot menu=off \ -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x7 \ --device usb-ccid,id=ccid0,bus=usb.0,port=1.1 \ -device usb-hub,id=hub0,bus=usb.0,port=1 \ +-device usb-ccid,id=ccid0,bus=usb.0,port=1.1 \ -drive file=/tmp/fdr.img,format=raw,if=none,id=drive-virtio-disk0,cache=none,\ aio=native \ -device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk0,\ diff --git a/tests/qemuxml2argvdata/name-escape.args b/tests/qemuxml2argvdata/name-escape.args index 58f284014c..d314ed409a 100644 --- a/tests/qemuxml2argvdata/name-escape.args +++ b/tests/qemuxml2argvdata/name-escape.args @@ -22,8 +22,8 @@ bar=2/monitor.sock,server,nowait \ -no-shutdown \ -no-acpi \ -device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 \ --device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -usb \ +-device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -drive file=/dev/HostVG/QEMUGuest1,format=qcow2,if=none,id=drive-ide0-0-0,\ cache=none,throttling.bps-total=5000,throttling.iops-total=6000,\ throttling.bps-total-max=10000,throttling.iops-total-max=11000,\ diff --git a/tests/qemuxml2argvdata/smartcard-controller.args b/tests/qemuxml2argvdata/smartcard-controller.args index 1a654cb4e1..4c5286dd87 100644 --- a/tests/qemuxml2argvdata/smartcard-controller.args +++ b/tests/qemuxml2argvdata/smartcard-controller.args @@ -20,7 +20,7 @@ server,nowait \ -rtc base=utc \ -no-shutdown \ -no-acpi \ --device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -usb \ +-device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -device ccid-card-emulated,backend=nss-emulated,id=smartcard0,bus=ccid0.0 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/smartcard-host-certificates-database.args b/tests/qemuxml2argvdata/smartcard-host-certificates-database.args index 17fb50699a..1056f52cfa 100644 --- a/tests/qemuxml2argvdata/smartcard-host-certificates-database.args +++ b/tests/qemuxml2argvdata/smartcard-host-certificates-database.args @@ -20,8 +20,8 @@ server,nowait \ -rtc base=utc \ -no-shutdown \ -no-acpi \ --device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -usb \ +-device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -device ccid-card-emulated,backend=certificates,cert1=cert1,cert2=cert2,\ cert3=cert3,db=/tmp/foo,id=smartcard0,bus=ccid0.0 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/smartcard-host-certificates.args b/tests/qemuxml2argvdata/smartcard-host-certificates.args index 489299a475..6bfb56b94b 100644 --- a/tests/qemuxml2argvdata/smartcard-host-certificates.args +++ b/tests/qemuxml2argvdata/smartcard-host-certificates.args @@ -20,8 +20,8 @@ server,nowait \ -rtc base=utc \ -no-shutdown \ -no-acpi \ --device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -usb \ +-device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -device ccid-card-emulated,backend=certificates,cert1=cert1,cert2=cert2,\ cert3=cert3,db=/etc/pki/nssdb,id=smartcard0,bus=ccid0.0 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/smartcard-host.args b/tests/qemuxml2argvdata/smartcard-host.args index 1a654cb4e1..4c5286dd87 100644 --- a/tests/qemuxml2argvdata/smartcard-host.args +++ b/tests/qemuxml2argvdata/smartcard-host.args @@ -20,7 +20,7 @@ server,nowait \ -rtc base=utc \ -no-shutdown \ -no-acpi \ --device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -usb \ +-device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -device ccid-card-emulated,backend=nss-emulated,id=smartcard0,bus=ccid0.0 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/smartcard-passthrough-spicevmc.args b/tests/qemuxml2argvdata/smartcard-passthrough-spicevmc.args index 8784a3ffaa..9fbc86bce7 100644 --- a/tests/qemuxml2argvdata/smartcard-passthrough-spicevmc.args +++ b/tests/qemuxml2argvdata/smartcard-passthrough-spicevmc.args @@ -20,8 +20,8 @@ server,nowait \ -rtc base=utc \ -no-shutdown \ -no-acpi \ --device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -usb \ +-device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -chardev spicevmc,id=charsmartcard0,name=smartcard \ -device ccid-card-passthru,chardev=charsmartcard0,id=smartcard0,bus=ccid0.0 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/smartcard-passthrough-tcp.args b/tests/qemuxml2argvdata/smartcard-passthrough-tcp.args index e55126c206..12c03ad4b6 100644 --- a/tests/qemuxml2argvdata/smartcard-passthrough-tcp.args +++ b/tests/qemuxml2argvdata/smartcard-passthrough-tcp.args @@ -20,8 +20,8 @@ server,nowait \ -rtc base=utc \ -no-shutdown \ -no-acpi \ --device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -usb \ +-device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -chardev socket,id=charsmartcard0,host=127.0.0.1,port=2001,server,nowait \ -device ccid-card-passthru,chardev=charsmartcard0,id=smartcard0,bus=ccid0.0 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/user-aliases.args b/tests/qemuxml2argvdata/user-aliases.args index 089a7d8d4b..7fd3b4a6e1 100644 --- a/tests/qemuxml2argvdata/user-aliases.args +++ b/tests/qemuxml2argvdata/user-aliases.args @@ -35,9 +35,9 @@ server,nowait \ -global PIIX4_PM.disable_s3=0 \ -global PIIX4_PM.disable_s4=0 \ -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x8 \ +-usb \ -device usb-ccid,id=ua-myCCID,bus=ua-SomeWeirdController.0,port=1 \ -device usb-ccid,id=ua-myCCID2,bus=ua-SomeWeirdController.0,port=2 \ --usb \ -drive file=/var/lib/libvirt/images/fd.img,format=raw,if=none,\ id=drive-ua-myDisk1,cache=none \ -drive file=/var/lib/libvirt/images/gentoo.qcow2,format=qcow2,if=none,\ -- 2.20.1
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_command.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7aafe235e2..5c7e53bb6a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3117,12 +3117,12 @@ qemuBuildControllersByTypeCommandLine(virCommandPtr cmd, virQEMUCapsPtr qemuCaps, virDomainControllerType type) { + char *devstr = NULL; int ret = -1; size_t i; for (i = 0; i < def->ncontrollers; i++) { virDomainControllerDefPtr cont = def->controllers[i]; - char *devstr; if (cont->type != type) continue; @@ -3155,23 +3155,22 @@ qemuBuildControllersByTypeCommandLine(virCommandPtr cmd, continue; } + VIR_FREE(devstr); if (qemuBuildControllerDevStr(def, cont, qemuCaps, &devstr) < 0) goto cleanup; if (devstr) { - if (qemuCommandAddExtDevice(cmd, &cont->info) < 0) { - VIR_FREE(devstr); + if (qemuCommandAddExtDevice(cmd, &cont->info) < 0) goto cleanup; - } virCommandAddArg(cmd, "-device"); virCommandAddArg(cmd, devstr); - VIR_FREE(devstr); } } ret = 0; cleanup: + VIR_FREE(devstr); return ret; } -- 2.20.1
Use qemuBuildControllersCommandLine since it builds the command line for (nearly) all controllers, not just one. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_command.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5c7e53bb6a..214d4068e9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3176,9 +3176,9 @@ qemuBuildControllersByTypeCommandLine(virCommandPtr cmd, static int -qemuBuildControllerDevCommandLine(virCommandPtr cmd, - const virDomainDef *def, - virQEMUCapsPtr qemuCaps) +qemuBuildControllersCommandLine(virCommandPtr cmd, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) { size_t j; int contOrder[] = { @@ -10597,7 +10597,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildGlobalControllerCommandLine(cmd, def, qemuCaps) < 0) goto error; - if (qemuBuildControllerDevCommandLine(cmd, def, qemuCaps) < 0) + if (qemuBuildControllersCommandLine(cmd, def, qemuCaps) < 0) goto error; if (qemuBuildHubCommandLine(cmd, def, qemuCaps) < 0) -- 2.20.1
Now that the nested loop is gone. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_command.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 214d4068e9..093a528bec 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3180,7 +3180,7 @@ qemuBuildControllersCommandLine(virCommandPtr cmd, const virDomainDef *def, virQEMUCapsPtr qemuCaps) { - size_t j; + size_t i; int contOrder[] = { /* * List of controller types that we add commandline args for, @@ -3209,8 +3209,8 @@ qemuBuildControllersCommandLine(virCommandPtr cmd, }; int ret = -1; - for (j = 0; j < ARRAY_CARDINALITY(contOrder); j++) { - if (qemuBuildControllersByTypeCommandLine(cmd, def, qemuCaps, contOrder[j]) < 0) + for (i = 0; i < ARRAY_CARDINALITY(contOrder); i++) { + if (qemuBuildControllersByTypeCommandLine(cmd, def, qemuCaps, contOrder[i]) < 0) goto cleanup; } -- 2.20.1
On 01/16/2019 02:56 AM, Ján Tomko wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1599971
Ján Tomko (11): qemu: move out legacy USB controller formatting qemu: exit early if USB_CONTROLLER_MODEL_NONE is present Add qemuBuildDomainForbidLegacyUSBController qemu: separate counting of legacy USB controllers qemu: separate counting of USB controllers qemuBuildControllerDevStr: remove nusbcontroller argument qemu: Introduce qemuBuildControllersByTypeCommandLine qemu: format CCID controllers after USB hubs qemuBuildControllersByTypeCommandLine: free devstr in the cleanup section rename qemuBuildControllerDevCommandLine qemuBuildControllersCommandLine: use i instead of j as the counter
src/qemu/qemu_command.c | 222 +++++++++++------- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_hotplug.c | 2 +- tests/qemuxml2argvdata/chardev-reconnect.args | 2 +- tests/qemuxml2argvdata/controller-order.args | 2 +- tests/qemuxml2argvdata/name-escape.args | 2 +- .../smartcard-controller.args | 2 +- .../smartcard-host-certificates-database.args | 2 +- .../smartcard-host-certificates.args | 2 +- tests/qemuxml2argvdata/smartcard-host.args | 2 +- .../smartcard-passthrough-spicevmc.args | 2 +- .../smartcard-passthrough-tcp.args | 2 +- tests/qemuxml2argvdata/user-aliases.args | 2 +- 13 files changed, 150 insertions(+), 97 deletions(-)
Reviewed-by: Cole Robinson <crobinso@redhat.com> Nice cleanups. Would be nice to move that 'Multiple legacy' error to Validate time, but that can come later Thanks, Cole
participants (2)
-
Cole Robinson -
Ján Tomko