[libvirt] [PATCH] qemu: Add controllers in specified order
qemu is sensitive to the order of arguments passed. Hence, if a device requires a controller, the controller cmd string must precede device cmd string. The same apply for controllers, when for instance ccid controller requires usb controller. So controllers create partial ordering in which they should be added to qemu cmd line. --- The order is basically random for now, with one constraint: CCID must be preceded with USB. src/qemu/qemu_command.c | 102 +++++++++++++++++++++++++--------------------- 1 files changed, 55 insertions(+), 47 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index fe99f5d..ffeb965 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4429,7 +4429,7 @@ qemuBuildCommandLine(virConnectPtr conn, virDomainSnapshotObjPtr snapshot, enum virNetDevVPortProfileOp vmop) { - int i; + int i, j; struct utsname ut; int disableKQEMU = 0; int enableKQEMU = 0; @@ -4446,6 +4446,17 @@ qemuBuildCommandLine(virConnectPtr conn, int usbcontroller = 0; bool usblegacy = false; uname_normalize(&ut); + int contOrder[] = { + /* We don't add an explicit IDE or FD controller because the + * provided PIIX4 device already includes one. It isn't possible to + * remove the PIIX4. */ + VIR_DOMAIN_CONTROLLER_TYPE_USB, + VIR_DOMAIN_CONTROLLER_TYPE_SCSI, + VIR_DOMAIN_CONTROLLER_TYPE_SATA, + VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL, + VIR_DOMAIN_CONTROLLER_TYPE_CCID, + }; + VIR_DEBUG("conn=%p driver=%p def=%p mon=%p json=%d " "caps=%p migrateFrom=%s migrateFD=%d " @@ -5024,61 +5035,59 @@ qemuBuildCommandLine(virConnectPtr conn, } if (qemuCapsGet(caps, QEMU_CAPS_DEVICE)) { - for (i = 0 ; i < def->ncontrollers ; i++) { - virDomainControllerDefPtr cont = def->controllers[i]; + for (j = 0; j < ARRAY_CARDINALITY(contOrder); j++) { + for (i = 0; i < def->ncontrollers; i++) { + virDomainControllerDefPtr cont = def->controllers[i]; - /* We don't add an explicit IDE or FD controller because the - * provided PIIX4 device already includes one. It isn't possible to - * remove the PIIX4. */ - if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE || - cont->type == VIR_DOMAIN_CONTROLLER_TYPE_FDC) - continue; + if (cont->type != contOrder[j]) + continue; - /* Also, 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; - } + /* Also, 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; + } - /* Only recent QEMU implements a SATA (AHCI) controller */ - if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA) { - if (!qemuCapsGet(caps, QEMU_CAPS_ICH9_AHCI)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("SATA is not supported with this " - "QEMU binary")); - goto error; + /* Only recent QEMU implements a SATA (AHCI) controller */ + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA) { + if (!qemuCapsGet(caps, QEMU_CAPS_ICH9_AHCI)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("SATA is not supported with this " + "QEMU binary")); + goto error; + } else { + char *devstr; + + virCommandAddArg(cmd, "-device"); + if (!(devstr = qemuBuildControllerDevStr(def, cont, + caps, NULL))) + goto error; + + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); + } + } else if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && + cont->model == -1 && + !qemuCapsGet(caps, QEMU_CAPS_PIIX3_USB_UHCI)) { + if (usblegacy) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Multiple legacy USB controllers are " + "not supported")); + goto error; + } + usblegacy = true; } else { - char *devstr; - virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildControllerDevStr(def, cont, - caps, NULL))) + + char *devstr; + if (!(devstr = qemuBuildControllerDevStr(def, cont, caps, + &usbcontroller))) goto error; virCommandAddArg(cmd, devstr); VIR_FREE(devstr); } - } else if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && - cont->model == -1 && - !qemuCapsGet(caps, QEMU_CAPS_PIIX3_USB_UHCI)) { - if (usblegacy) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Multiple legacy USB controllers are " - "not supported")); - goto error; - } - usblegacy = true; - } else { - virCommandAddArg(cmd, "-device"); - - char *devstr; - if (!(devstr = qemuBuildControllerDevStr(def, cont, caps, - &usbcontroller))) - goto error; - - virCommandAddArg(cmd, devstr); - VIR_FREE(devstr); } } } @@ -5553,7 +5562,6 @@ qemuBuildCommandLine(virConnectPtr conn, virDomainSmartcardDefPtr smartcard = def->smartcards[0]; char *devstr; virBuffer opt = VIR_BUFFER_INITIALIZER; - int j; const char *database; if (def->nsmartcards > 1 || -- 1.7.8.6
On 11/01/2012 08:11 AM, Michal Privoznik wrote:
qemu is sensitive to the order of arguments passed. Hence, if a device requires a controller, the controller cmd string must precede device cmd string. The same apply for controllers, when for instance ccid controller requires usb controller. So controllers create partial ordering in which they should be added to qemu cmd line. ---
The order is basically random for now, with one constraint: CCID must be preceded with USB.
src/qemu/qemu_command.c | 102 +++++++++++++++++++++++++--------------------- 1 files changed, 55 insertions(+), 47 deletions(-)
Shouldn't we have at least one test under tests/qemuxml2argvdata that is impacted by this new ordering? And if not, we should add such a test. The patch is a bit hard to read, because it reindents at the same time as adding only a couple lines of code. I find it easier to split these types of patches into two - one that adds the new code but doesn't reindent, and a followup that fixes indentation; or the reverse order of adding a {} group to reindent with no semantic change, followed by the patch that adds the actual feature. That said, the code looks right, but I'd still like to see a testcase proving it before giving ack. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Michal Privoznik