[libvirt] [PATCH 00/17] Move qemu command line controller checks to qemuDomainDeviceDefValidateController* checks
Recent series to just move IDE checks: (v4: https://www.redhat.com/archives/libvir-list/2017-December/msg00049.html) gave me enough "push" in order to move the bulk of qemu_command controller command line build "validation" checks into their own Validation helpers. The IDE only checks are essentially incorporated into this series. John Ferlan (14): qemu: Introduce qemuDomainDeviceDefValidateController qemu: Introduce qemuDomainDeviceDefSkipController qemu: Use virDomainControllerType in qemuBuildControllerDevStr switch qemu: Move CCW S390 Address check to controller def validate qemu: Introduce qemuDomainDeviceDefValidateControllerSCSI qemu: Introduce qemuDomainDeviceDefValidateControllerPCI qemu: Use virDomainPCIControllerOpts in qemuBuildControllerDevStr qemu: Move PCI command modelName check to controller def validate qemu: Move PCI command modelName TypeToString to controller def validate qemu: Move PCI more command checks to controller def validate qemu: Complete PCI command checks to controller def validate qemu: Introduce qemuDomainDeviceDefValidateControllerSATA qemu: Introduce qemuDomainDeviceDefValidateControllerUSB qemu: Complete move USB command checks to controller def validate Lin Ma (3): tests: Remove use of IDE disk for pseries floppy test tests: Drop IDE controller in CCW qemu: Introduce qemuDomainDeviceDefValidateControllerIDE src/qemu/qemu_command.c | 566 ++------------------ src/qemu/qemu_command.h | 2 + src/qemu/qemu_domain.c | 590 ++++++++++++++++++++- src/qemu/qemu_domain.h | 12 + .../qemuhotplug-base-ccw-live+ccw-virtio.xml | 4 - ...ive-with-2-ccw-virtio+ccw-virtio-1-explicit.xml | 4 - ...live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml | 4 - ...qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml | 4 - ...-live-with-ccw-virtio+ccw-virtio-2-explicit.xml | 4 - ...-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml | 4 - .../qemuhotplug-base-ccw-live-with-ccw-virtio.xml | 4 - .../qemuhotplug-base-ccw-live.xml | 4 - tests/qemumemlocktest.c | 14 + .../qemuxml2argv-disk-floppy-pseries.args | 24 - .../qemuxml2argv-disk-floppy-pseries.xml | 7 - tests/qemuxml2argvtest.c | 18 +- tests/qemuxml2xmltest.c | 20 +- 17 files changed, 689 insertions(+), 596 deletions(-) delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-floppy-pseries.args -- 2.13.6
Introduce the bare bones helper to validate whether the controller definition is valid. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e8e03134f..569bbd29e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3885,6 +3885,27 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk) static int +qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, + const virDomainDef *def ATTRIBUTE_UNUSED) +{ + switch ((virDomainControllerType) controller->type) { + case VIR_DOMAIN_CONTROLLER_TYPE_IDE: + case VIR_DOMAIN_CONTROLLER_TYPE_FDC: + case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: + case VIR_DOMAIN_CONTROLLER_TYPE_SATA: + case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: + case VIR_DOMAIN_CONTROLLER_TYPE_CCID: + case VIR_DOMAIN_CONTROLLER_TYPE_USB: + case VIR_DOMAIN_CONTROLLER_TYPE_PCI: + case VIR_DOMAIN_CONTROLLER_TYPE_LAST: + break; + } + + return 0; +} + + +static int qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, const virDomainDef *def, void *opaque ATTRIBUTE_UNUSED) @@ -3928,11 +3949,14 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, ret = qemuDomainDeviceDefValidateDisk(dev->data.disk); break; + case VIR_DOMAIN_DEVICE_CONTROLLER: + ret = qemuDomainDeviceDefValidateController(dev->data.controller, def); + break; + case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: - case VIR_DOMAIN_DEVICE_CONTROLLER: case VIR_DOMAIN_DEVICE_GRAPHICS: case VIR_DOMAIN_DEVICE_HUB: case VIR_DOMAIN_DEVICE_MEMBALLOON: -- 2.13.6
In order to be able to reuse some code for both controller def validation and command line building, create a convenience helper that will perform the "skip" avoidance checks. It will also set some flags to allow the caller to specifically skip (or fail) depending on the result of the flag (as opposed to building up some ever growing list of variables). Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 61 +++++------------------------------ src/qemu/qemu_domain.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_domain.h | 12 +++++++ 3 files changed, 102 insertions(+), 55 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6a8da1d58..5a6a671a1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3180,8 +3180,8 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, virQEMUCapsPtr qemuCaps) { size_t i, j; + unsigned int flags = 0; int usbcontroller = 0; - bool usblegacy = false; int contOrder[] = { /* * List of controller types that we add commandline args for, @@ -3216,61 +3216,14 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, if (cont->type != contOrder[j]) continue; - /* 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; - } - - /* skip pcie-root */ - if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && - cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) { - continue; - } + if (qemuDomainDeviceDefSkipController(cont, def, &flags)) { + /* mark we don't want a controller */ + if (flags & QEMU_DOMAIN_DEVICE_SKIP_USB_CONTROLLER_FLAG) + usbcontroller = -1; - /* Skip pci-root, except for pSeries guests (which actually - * support more than one PCI Host Bridge per guest) */ - if (!qemuDomainIsPSeries(def) && - cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && - cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) { - continue; - } - - /* first SATA controller on Q35 machines is implicit */ - if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA && - cont->idx == 0 && qemuDomainIsQ35(def)) - continue; - - /* first IDE controller is implicit on various machines */ - if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && - cont->idx == 0 && qemuDomainHasBuiltinIDE(def)) - continue; - - if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && - cont->model == -1 && - !qemuDomainIsQ35(def) && - !qemuDomainIsVirt(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 */ - if (usblegacy) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Multiple legacy USB controllers are " - "not supported")); + if (flags & QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FAIL_FLAG) goto cleanup; - } - usblegacy = true; + continue; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 569bbd29e..d4c7674c0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3884,10 +3884,92 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk) } +/** + * qemuDomainDeviceDefSkipController: + * @controller: Controller to check + * @def: Domain definition + * @flags: qemuDomainDeviceSkipFlags to set if specific condition found + * + * Returns true if this controller can be skipped for command line generation + * or device validation + */ +bool +qemuDomainDeviceDefSkipController(const virDomainControllerDef *controller, + const virDomainDef *def, + unsigned int *flags) +{ + /* skip USB controllers with type none.*/ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) { + *flags |= QEMU_DOMAIN_DEVICE_SKIP_USB_CONTROLLER_FLAG; + return true; + } + + /* skip pcie-root */ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) + return true; + + /* Skip pci-root, except for pSeries guests (which actually + * support more than one PCI Host Bridge per guest) */ + if (!qemuDomainIsPSeries(def) && + controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) + return true; + + /* first SATA controller on Q35 machines is implicit */ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA && + controller->idx == 0 && qemuDomainIsQ35(def)) + return true; + + /* first IDE controller is implicit on various machines */ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && + controller->idx == 0 && qemuDomainHasBuiltinIDE(def)) + return true; + + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && + controller->model == -1 && + !qemuDomainIsQ35(def) && + !qemuDomainIsVirt(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 */ + if (*flags & QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FLAG) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Multiple legacy USB controllers are " + "not supported")); + *flags |= QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FAIL_FLAG; + } + *flags |= QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FLAG; + return true; + } + + return false; +} + + static int qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, - const virDomainDef *def ATTRIBUTE_UNUSED) + const virDomainDef *def) { + unsigned int flags = 0; + + if (qemuDomainDeviceDefSkipController(controller, def, &flags)) + return 0; + + if (flags & QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FAIL_FLAG) + return -1; + switch ((virDomainControllerType) controller->type) { case VIR_DOMAIN_CONTROLLER_TYPE_IDE: case VIR_DOMAIN_CONTROLLER_TYPE_FDC: diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index c33af3671..5c9c55d38 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -999,4 +999,16 @@ qemuDomainPrepareDiskSource(virConnectPtr conn, qemuDomainObjPrivatePtr priv, virQEMUDriverConfigPtr cfg); +typedef enum { + QEMU_DOMAIN_DEVICE_SKIP_USB_CONTROLLER_FLAG = (1 << 0), + QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FLAG = (1 << 1), + QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FAIL_FLAG = (1 << 2), +} qemuDomainDeviceSkipFlags; + +bool +qemuDomainDeviceDefSkipController(const virDomainControllerDef *controller, + const virDomainDef *def, + unsigned int *flags); + + #endif /* __QEMU_DOMAIN_H__ */ -- 2.13.6
On Mon, Dec 04, 2017 at 08:38:52PM -0500, John Ferlan wrote:
In order to be able to reuse some code for both controller def validation and command line building, create a convenience helper that will perform the "skip" avoidance checks. It will also set some flags to allow the caller to specifically skip (or fail) depending on the result of the flag (as opposed to building up some ever growing list of variables).
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 61 +++++------------------------------ src/qemu/qemu_domain.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_domain.h | 12 +++++++ 3 files changed, 102 insertions(+), 55 deletions(-)
[...]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 569bbd29e..d4c7674c0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3884,10 +3884,92 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk) }
+/** + * qemuDomainDeviceDefSkipController: + * @controller: Controller to check + * @def: Domain definition + * @flags: qemuDomainDeviceSkipFlags to set if specific condition found + * + * Returns true if this controller can be skipped for command line generation + * or device validation
We should not skip validation for implicit devices. Some of the checks added later are for implicit devices (PCI root, SATA controller). It would be enough to split out the logic of figuring out whether the controller is implicit out of command line generation.
+ */ +bool +qemuDomainDeviceDefSkipController(const virDomainControllerDef *controller, + const virDomainDef *def, + unsigned int *flags) +{ + /* skip USB controllers with type none.*/ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) { + *flags |= QEMU_DOMAIN_DEVICE_SKIP_USB_CONTROLLER_FLAG; + return true; + }
Simply returning true here (no USB controller is implicit) should suffice. The caller can figure out whether USB_NONE is present by going through the controllers array again (via virDomainDefHasUSB).
+ + /* skip pcie-root */ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) + return true; + + /* Skip pci-root, except for pSeries guests (which actually + * support more than one PCI Host Bridge per guest) */ + if (!qemuDomainIsPSeries(def) && + controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) + return true; + + /* first SATA controller on Q35 machines is implicit */ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA && + controller->idx == 0 && qemuDomainIsQ35(def)) + return true; + + /* first IDE controller is implicit on various machines */ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && + controller->idx == 0 && qemuDomainHasBuiltinIDE(def)) + return true; + + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && + controller->model == -1 && + !qemuDomainIsQ35(def) && + !qemuDomainIsVirt(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 */ + if (*flags & QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FLAG) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Multiple legacy USB controllers are " + "not supported"));
A function returning bool where both options are valid should not be setting errors. For this error, validate can go through all the controllers. Command line generator should not care and we can get rid of the remaining two flags as their only purpose is to save us multiple passes through the controllers field.
+ *flags |= QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FAIL_FLAG; + } + *flags |= QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FLAG; + return true; + } + + return false; +} + + static int qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, - const virDomainDef *def ATTRIBUTE_UNUSED) + const virDomainDef *def) { + unsigned int flags = 0; + + if (qemuDomainDeviceDefSkipController(controller, def, &flags)) + return 0; + + if (flags & QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FAIL_FLAG) + return -1;
To possibly set this flag, SkipController must be called with non-zero flags, so this would be dead code. I'd rather go through def->controllers again to look for another legacy controller, just for the purpose of validation. Jan
+ switch ((virDomainControllerType) controller->type) { case VIR_DOMAIN_CONTROLLER_TYPE_IDE: case VIR_DOMAIN_CONTROLLER_TYPE_FDC: diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index c33af3671..5c9c55d38 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -999,4 +999,16 @@ qemuDomainPrepareDiskSource(virConnectPtr conn, qemuDomainObjPrivatePtr priv, virQEMUDriverConfigPtr cfg);
+typedef enum { + QEMU_DOMAIN_DEVICE_SKIP_USB_CONTROLLER_FLAG = (1 << 0), + QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FLAG = (1 << 1), + QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FAIL_FLAG = (1 << 2), +} qemuDomainDeviceSkipFlags; + +bool +qemuDomainDeviceDefSkipController(const virDomainControllerDef *controller, + const virDomainDef *def, + unsigned int *flags); + + #endif /* __QEMU_DOMAIN_H__ */ -- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 12/05/2017 09:18 AM, Ján Tomko wrote:
On Mon, Dec 04, 2017 at 08:38:52PM -0500, John Ferlan wrote:
In order to be able to reuse some code for both controller def validation and command line building, create a convenience helper that will perform the "skip" avoidance checks. It will also set some flags to allow the caller to specifically skip (or fail) depending on the result of the flag (as opposed to building up some ever growing list of variables).
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 61 +++++------------------------------ src/qemu/qemu_domain.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_domain.h | 12 +++++++ 3 files changed, 102 insertions(+), 55 deletions(-)
[...]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 569bbd29e..d4c7674c0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3884,10 +3884,92 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk) }
+/** + * qemuDomainDeviceDefSkipController: + * @controller: Controller to check + * @def: Domain definition + * @flags: qemuDomainDeviceSkipFlags to set if specific condition found + * + * Returns true if this controller can be skipped for command line generation + * or device validation
We should not skip validation for implicit devices. Some of the checks added later are for implicit devices (PCI root, SATA controller).
It would be enough to split out the logic of figuring out whether the controller is implicit out of command line generation.
Obviously the "goal" was to avoid the same checks in Validation that we're avoiding in command line building; however, if there are things that need to be checked in Validation before what gets checked here, then I could see that being "extra". What I was trying to avoid was duplication of specific checks prior to Validation that were being avoided anyways for command line building. As in - we don't build that on the command line, so why bother checking during validation.
+ */ +bool +qemuDomainDeviceDefSkipController(const virDomainControllerDef *controller, + const virDomainDef *def, + unsigned int *flags) +{ + /* skip USB controllers with type none.*/ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) { + *flags |= QEMU_DOMAIN_DEVICE_SKIP_USB_CONTROLLER_FLAG; + return true; + }
Simply returning true here (no USB controller is implicit) should suffice. The caller can figure out whether USB_NONE is present by going through the controllers array again (via virDomainDefHasUSB).
+ + /* skip pcie-root */ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) + return true; + + /* Skip pci-root, except for pSeries guests (which actually + * support more than one PCI Host Bridge per guest) */ + if (!qemuDomainIsPSeries(def) && + controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) + return true; + + /* first SATA controller on Q35 machines is implicit */ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA && + controller->idx == 0 && qemuDomainIsQ35(def)) + return true; + + /* first IDE controller is implicit on various machines */ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && + controller->idx == 0 && qemuDomainHasBuiltinIDE(def)) + return true;
This one is the initial reason I figured it would be better to have a Skip helper as opposed to "copying" the same check in the ValidateIDE code that we'd have in the command line building code. Then the subsequent ones made the ValidateUSB *so much* easier.
+ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && + controller->model == -1 && + !qemuDomainIsQ35(def) && + !qemuDomainIsVirt(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 */ + if (*flags & QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FLAG) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Multiple legacy USB controllers are " + "not supported"));
A function returning bool where both options are valid should not be setting errors.
Yeah - I figured this may cause a few eyebrows to raise.
For this error, validate can go through all the controllers. Command line generator should not care and we can get rid of the remaining two flags as their only purpose is to save us multiple passes through the controllers field.
+ *flags |= QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FAIL_FLAG; + } + *flags |= QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FLAG; + return true; + } + + return false; +} + + static int qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, - const virDomainDef *def ATTRIBUTE_UNUSED) + const virDomainDef *def) { + unsigned int flags = 0; + + if (qemuDomainDeviceDefSkipController(controller, def, &flags)) + return 0; + + if (flags & QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FAIL_FLAG) + return -1;
To possibly set this flag, SkipController must be called with non-zero flags, so this would be dead code.
I'd rather go through def->controllers again to look for another legacy controller, just for the purpose of validation.
OK - I'll try to figure out some sort of happy medium. It obviously could affect subsequent patches in the series. John
Jan
+ switch ((virDomainControllerType) controller->type) { case VIR_DOMAIN_CONTROLLER_TYPE_IDE: case VIR_DOMAIN_CONTROLLER_TYPE_FDC: diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index c33af3671..5c9c55d38 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -999,4 +999,16 @@ qemuDomainPrepareDiskSource(virConnectPtr conn, qemuDomainObjPrivatePtr priv, virQEMUDriverConfigPtr cfg);
+typedef enum { + QEMU_DOMAIN_DEVICE_SKIP_USB_CONTROLLER_FLAG = (1 << 0), + QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FLAG = (1 << 1), + QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FAIL_FLAG = (1 << 2), +} qemuDomainDeviceSkipFlags; + +bool +qemuDomainDeviceDefSkipController(const virDomainControllerDef *controller, + const virDomainDef *def, + unsigned int *flags); + + #endif /* __QEMU_DOMAIN_H__ */ -- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 12/05/2017 09:18 AM, Ján Tomko wrote:
On Mon, Dec 04, 2017 at 08:38:52PM -0500, John Ferlan wrote:
In order to be able to reuse some code for both controller def validation and command line building, create a convenience helper that will perform the "skip" avoidance checks. It will also set some flags to allow the caller to specifically skip (or fail) depending on the result of the flag (as opposed to building up some ever growing list of variables).
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 61 +++++------------------------------ src/qemu/qemu_domain.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_domain.h | 12 +++++++ 3 files changed, 102 insertions(+), 55 deletions(-)
[...]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 569bbd29e..d4c7674c0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3884,10 +3884,92 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk) }
+/** + * qemuDomainDeviceDefSkipController: + * @controller: Controller to check + * @def: Domain definition + * @flags: qemuDomainDeviceSkipFlags to set if specific condition found + * + * Returns true if this controller can be skipped for command line generation + * or device validation
We should not skip validation for implicit devices. Some of the checks added later are for implicit devices (PCI root, SATA controller).
It would be enough to split out the logic of figuring out whether the controller is implicit out of command line generation.
After some more thought - I'm not sure it's really clear to me what you're driving at. The whole point of the Skip helper was to avoid duplicating checks in specific ValidateController{IDE|PCI|SATA|USB} helpers. If we're not building a command line for those, then what is the purpose of going through Validation? I suppose I could duplicate the same checks in the helpers, but that didn't feel right. If I take the IDE check as an example, the IDE validation would need code like Lin Ma's patches had, e.g.: if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) { /* No need to check implicit/builtin IDE controller */ if (controller->idx == 0 && qemuDomainHasBuiltinIDE(def)) return 0; ... Personally, trying to reduce cut-copy-paste would be a gain.
+ */ +bool +qemuDomainDeviceDefSkipController(const virDomainControllerDef *controller, + const virDomainDef *def, + unsigned int *flags) +{ + /* skip USB controllers with type none.*/ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) { + *flags |= QEMU_DOMAIN_DEVICE_SKIP_USB_CONTROLLER_FLAG; + return true; + }
Simply returning true here (no USB controller is implicit) should suffice. The caller can figure out whether USB_NONE is present by going through the controllers array again (via virDomainDefHasUSB).
I'm just going to drop moving the USB checks into ValidateControllerUSB - it seems you have an idea of what you'd like to see done and I'm fine with reviewing that when/if it shows up on the list. The whole reason for the flags argument was because of the very specific USB checks being done in command line building. Since you clearly understand those better than I do, for me to make progress it's best to just defer to you. Updated series to be sent shortly... John
+ + /* skip pcie-root */ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) + return true; + + /* Skip pci-root, except for pSeries guests (which actually + * support more than one PCI Host Bridge per guest) */ + if (!qemuDomainIsPSeries(def) && + controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) + return true; + + /* first SATA controller on Q35 machines is implicit */ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA && + controller->idx == 0 && qemuDomainIsQ35(def)) + return true; + + /* first IDE controller is implicit on various machines */ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && + controller->idx == 0 && qemuDomainHasBuiltinIDE(def)) + return true; + + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && + controller->model == -1 && + !qemuDomainIsQ35(def) && + !qemuDomainIsVirt(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 */ + if (*flags & QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FLAG) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Multiple legacy USB controllers are " + "not supported"));
A function returning bool where both options are valid should not be setting errors.
For this error, validate can go through all the controllers. Command line generator should not care and we can get rid of the remaining two flags as their only purpose is to save us multiple passes through the controllers field.
+ *flags |= QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FAIL_FLAG; + } + *flags |= QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FLAG; + return true; + } + + return false; +} + + static int qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, - const virDomainDef *def ATTRIBUTE_UNUSED) + const virDomainDef *def) { + unsigned int flags = 0; + + if (qemuDomainDeviceDefSkipController(controller, def, &flags)) + return 0; + + if (flags & QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FAIL_FLAG) + return -1;
To possibly set this flag, SkipController must be called with non-zero flags, so this would be dead code.
I'd rather go through def->controllers again to look for another legacy controller, just for the purpose of validation.
Jan
+ switch ((virDomainControllerType) controller->type) { case VIR_DOMAIN_CONTROLLER_TYPE_IDE: case VIR_DOMAIN_CONTROLLER_TYPE_FDC: diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index c33af3671..5c9c55d38 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -999,4 +999,16 @@ qemuDomainPrepareDiskSource(virConnectPtr conn, qemuDomainObjPrivatePtr priv, virQEMUDriverConfigPtr cfg);
+typedef enum { + QEMU_DOMAIN_DEVICE_SKIP_USB_CONTROLLER_FLAG = (1 << 0), + QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FLAG = (1 << 1), + QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FAIL_FLAG = (1 << 2), +} qemuDomainDeviceSkipFlags; + +bool +qemuDomainDeviceDefSkipController(const virDomainControllerDef *controller, + const virDomainDef *def, + unsigned int *flags); + + #endif /* __QEMU_DOMAIN_H__ */ -- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Dec 06, 2017 at 10:18:53AM -0500, John Ferlan wrote:
On 12/05/2017 09:18 AM, Ján Tomko wrote:
[...]
We should not skip validation for implicit devices. Some of the checks added later are for implicit devices (PCI root, SATA controller).
It would be enough to split out the logic of figuring out whether the controller is implicit out of command line generation.
After some more thought - I'm not sure it's really clear to me what you're driving at.
The whole point of the Skip helper was to avoid duplicating checks in specific ValidateController{IDE|PCI|SATA|USB} helpers. If we're not building a command line for those, then what is the purpose of going through Validation?
The purpose of qemuDomain*DefValidate is to validate the domain definition, not the command line. So it still makes sense to check if what is provided in the domain definition matches the implicit device.
I suppose I could duplicate the same checks in the helpers, but that didn't feel right. If I take the IDE check as an example, the IDE validation would need code like Lin Ma's patches had, e.g.:
if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) { /* No need to check implicit/builtin IDE controller */ if (controller->idx == 0 && qemuDomainHasBuiltinIDE(def)) return 0; ...
Personally, trying to reduce cut-copy-paste would be a gain.
Without this check, the IDE controller validation seemed a bit confusing, since it returned an error in all the cases, leaving up to the reader to figure out that the validate function is not called on valid devices. Also, since the pci(e)-root checks were skipped. Jan
+ */ +bool +qemuDomainDeviceDefSkipController(const virDomainControllerDef *controller, + const virDomainDef *def, + unsigned int *flags) +{ + /* skip USB controllers with type none.*/ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) { + *flags |= QEMU_DOMAIN_DEVICE_SKIP_USB_CONTROLLER_FLAG; + return true; + }
Simply returning true here (no USB controller is implicit) should suffice. The caller can figure out whether USB_NONE is present by going through the controllers array again (via virDomainDefHasUSB).
I'm just going to drop moving the USB checks into ValidateControllerUSB - it seems you have an idea of what you'd like to see done and I'm fine with reviewing that when/if it shows up on the list.
The whole reason for the flags argument was because of the very specific USB checks being done in command line building. Since you clearly understand those better than I do, for me to make progress it's best to just defer to you.
Updated series to be sent shortly...
John
Make sure all types of virDomainControllerType are handled in the switch statement. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5a6a671a1..818a057bc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2695,7 +2695,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, } } - switch (def->type) { + switch ((virDomainControllerType) def->type) { case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: switch (model) { case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: @@ -3140,7 +3140,8 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, "this QEMU binary or machine type")); goto error; - default: + case VIR_DOMAIN_CONTROLLER_TYPE_FDC: + case VIR_DOMAIN_CONTROLLER_TYPE_LAST: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unsupported controller type: %s"), virDomainControllerTypeToString(def->type)); -- 2.13.6
On Mon, Dec 04, 2017 at 08:38:53PM -0500, John Ferlan wrote:
Make sure all types of virDomainControllerType are handled in the switch statement.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
ACK Jan
Move the call to qemuDomainCheckCCWS390AddressSupport from qemuBuildControllerDevStr to qemuDomainDeviceDefValidateController. This means we will get the qemuCaps from the driver opaque variable passed to qemuDomainDeviceDefValidate. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 4 ---- src/qemu/qemu_domain.c | 18 +++++++++++++++--- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 818a057bc..15d9209c6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2662,10 +2662,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, *devstr = NULL; - if (!qemuDomainCheckCCWS390AddressSupport(domainDef, def->info, qemuCaps, - "controller")) - return -1; - if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { if ((qemuDomainSetSCSIControllerModel(domainDef, qemuCaps, &model)) < 0) return -1; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d4c7674c0..1fc360af9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3960,7 +3960,8 @@ qemuDomainDeviceDefSkipController(const virDomainControllerDef *controller, static int qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, - const virDomainDef *def) + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) { unsigned int flags = 0; @@ -3970,6 +3971,10 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, if (flags & QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FAIL_FLAG) return -1; + if (!qemuDomainCheckCCWS390AddressSupport(def, controller->info, qemuCaps, + "controller")) + return -1; + switch ((virDomainControllerType) controller->type) { case VIR_DOMAIN_CONTROLLER_TYPE_IDE: case VIR_DOMAIN_CONTROLLER_TYPE_FDC: @@ -3990,9 +3995,15 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, static int qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, const virDomainDef *def, - void *opaque ATTRIBUTE_UNUSED) + void *opaque) { int ret = 0; + virQEMUDriverPtr driver = opaque; + virQEMUCapsPtr qemuCaps = NULL; + + if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, + def->emulator))) + return -1; switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_NET: @@ -4032,7 +4043,8 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, break; case VIR_DOMAIN_DEVICE_CONTROLLER: - ret = qemuDomainDeviceDefValidateController(dev->data.controller, def); + ret = qemuDomainDeviceDefValidateController(dev->data.controller, def, + qemuCaps); break; case VIR_DOMAIN_DEVICE_LEASE: -- 2.13.6
On Mon, Dec 04, 2017 at 08:38:54PM -0500, John Ferlan wrote:
Move the call to qemuDomainCheckCCWS390AddressSupport from qemuBuildControllerDevStr to qemuDomainDeviceDefValidateController.
This means we will get the qemuCaps from the driver opaque variable passed to qemuDomainDeviceDefValidate.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 4 ---- src/qemu/qemu_domain.c | 18 +++++++++++++++--- 2 files changed, 15 insertions(+), 7 deletions(-)
@@ -3990,9 +3995,15 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, static int qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, const virDomainDef *def, - void *opaque ATTRIBUTE_UNUSED) + void *opaque) { int ret = 0; + virQEMUDriverPtr driver = opaque; + virQEMUCapsPtr qemuCaps = NULL; + + if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, + def->emulator))) + return -1;
The corresponding Unref should be a part of this patch Jan
switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_NET: @@ -4032,7 +4043,8 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, break;
case VIR_DOMAIN_DEVICE_CONTROLLER: - ret = qemuDomainDeviceDefValidateController(dev->data.controller, def); + ret = qemuDomainDeviceDefValidateController(dev->data.controller, def, + qemuCaps); break;
case VIR_DOMAIN_DEVICE_LEASE: -- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
From: Lin Ma <lma@suse.com> Adding an IDE controller for a machinetype that has no built-in IDE controller, libvirt will log an error. Currently the machinetype list which returns by qemuDomainMachineHasBuiltinIDE only includes 440fx, malta, sun4u and g3beige. Remove the disk and the .args file since the expectation is the test will fail in qemuxml2argvtest because floppy is not supported on pseries and thus no disk is necessary and no .args file would be created to compare against. Signed-off-by: Lin Ma <lma@suse.com> Signed-off-by: John Ferlan <jferlan@redhat.com> --- .../qemuxml2argv-disk-floppy-pseries.args | 24 ---------------------- .../qemuxml2argv-disk-floppy-pseries.xml | 7 ------- 2 files changed, 31 deletions(-) delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-floppy-pseries.args diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-floppy-pseries.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-floppy-pseries.args deleted file mode 100644 index 72a418302..000000000 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-floppy-pseries.args +++ /dev/null @@ -1,24 +0,0 @@ -LC_ALL=C \ -PATH=/bin \ -HOME=/home/test \ -USER=test \ -LOGNAME=test \ -QEMU_AUDIO_DRV=none \ -/usr/bin/qemu-system-ppc64 \ --name QEMUGuest1 \ --S \ --M pseries \ --m 214 \ --smp 1 \ --uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ --nographic \ --monitor unix:/tmp/test-monitor,server,nowait \ --no-acpi \ --boot c \ --usb \ --drive file=/dev/HostVG/QEMUGuest1,format=raw,if=ide,bus=0,unit=0 \ --drive file=/dev/fd0,format=raw,if=floppy,unit=0 \ --drive file=/tmp/firmware.img,format=raw,if=floppy,unit=1 \ --net none \ --serial none \ --parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-floppy-pseries.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-floppy-pseries.xml index be0ede6bd..a4191abe1 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-floppy-pseries.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-floppy-pseries.xml @@ -14,12 +14,6 @@ <on_crash>destroy</on_crash> <devices> <emulator>/usr/bin/qemu-system-ppc64</emulator> - <disk type='block' device='disk'> - <driver name='qemu' type='raw'/> - <source dev='/dev/HostVG/QEMUGuest1'/> - <target dev='hda' bus='ide'/> - <address type='drive' controller='0' bus='0' target='0' unit='0'/> - </disk> <disk type='block' device='floppy'> <driver name='qemu' type='raw'/> <source dev='/dev/fd0'/> @@ -34,7 +28,6 @@ </disk> <controller type='usb' index='0'/> <controller type='fdc' index='0'/> - <controller type='ide' index='0'/> <controller type='pci' index='0' model='pci-root'/> <memballoon model='none'/> </devices> -- 2.13.6
On Mon, Dec 04, 2017 at 08:38:55PM -0500, John Ferlan wrote:
From: Lin Ma <lma@suse.com>
Adding an IDE controller for a machinetype that has no built-in IDE controller, libvirt will log an error. Currently the machinetype list which returns by qemuDomainMachineHasBuiltinIDE only includes 440fx, malta, sun4u and g3beige.
Remove the disk and the .args file since the expectation is the test will fail in qemuxml2argvtest because floppy is not supported on pseries and thus no disk is necessary and no .args file would be created to compare against.
Signed-off-by: Lin Ma <lma@suse.com> Signed-off-by: John Ferlan <jferlan@redhat.com> --- .../qemuxml2argv-disk-floppy-pseries.args | 24 ---------------------- .../qemuxml2argv-disk-floppy-pseries.xml | 7 ------- 2 files changed, 31 deletions(-) delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-floppy-pseries.args
ACK Jan
From: Lin Ma <lma@suse.com> Adding an IDE controller for a machinetype that has no built-in IDE controller, libvirt will log an error. Currently the machinetype list which returns by qemuDomainMachineHasBuiltinIDE only includes 440fx, malta, sun4u and g3beige. Signed-off-by: Lin Ma <lma@suse.com> Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml | 4 ---- ...uhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-explicit.xml | 4 ---- ...muhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml | 4 ---- .../qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml | 4 ---- ...emuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml | 4 ---- .../qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml | 4 ---- .../qemuhotplug-base-ccw-live-with-ccw-virtio.xml | 4 ---- tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live.xml | 4 ---- 8 files changed, 32 deletions(-) diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml index cd03d0e09..75948f324 100644 --- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml @@ -33,10 +33,6 @@ <alias name='usb'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> </controller> - <controller type='ide' index='0'> - <alias name='ide0'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> - </controller> <controller type='scsi' index='0' model='virtio-scsi'> <alias name='scsi0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-explicit.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-explicit.xml index 519d8161f..301658178 100644 --- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-explicit.xml +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-explicit.xml @@ -43,10 +43,6 @@ <alias name='usb'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> </controller> - <controller type='ide' index='0'> - <alias name='ide0'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> - </controller> <controller type='scsi' index='0' model='virtio-scsi'> <alias name='scsi0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml index 7be75f977..bb9d427c2 100644 --- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml @@ -43,10 +43,6 @@ <alias name='usb'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> </controller> - <controller type='ide' index='0'> - <alias name='ide0'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> - </controller> <controller type='scsi' index='0' model='virtio-scsi'> <alias name='scsi0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml index a83f1b5d7..c41f5affd 100644 --- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml @@ -33,10 +33,6 @@ <alias name='usb'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> </controller> - <controller type='ide' index='0'> - <alias name='ide0'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> - </controller> <controller type='scsi' index='0' model='virtio-scsi'> <alias name='scsi0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml index 3e9020751..b1114dba9 100644 --- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml @@ -42,10 +42,6 @@ <alias name='usb'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> </controller> - <controller type='ide' index='0'> - <alias name='ide0'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> - </controller> <controller type='scsi' index='0' model='virtio-scsi'> <alias name='scsi0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml index 3e9020751..b1114dba9 100644 --- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml @@ -42,10 +42,6 @@ <alias name='usb'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> </controller> - <controller type='ide' index='0'> - <alias name='ide0'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> - </controller> <controller type='scsi' index='0' model='virtio-scsi'> <alias name='scsi0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio.xml index 0fa8d036b..3476f61db 100644 --- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio.xml +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio.xml @@ -32,10 +32,6 @@ <alias name='usb'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> </controller> - <controller type='ide' index='0'> - <alias name='ide0'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> - </controller> <controller type='scsi' index='0' model='virtio-scsi'> <alias name='scsi0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live.xml index d4434a18c..3a202d069 100644 --- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live.xml +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live.xml @@ -23,10 +23,6 @@ <alias name='usb'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> </controller> - <controller type='ide' index='0'> - <alias name='ide0'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> - </controller> <controller type='scsi' index='0' model='virtio-scsi'> <alias name='scsi0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> -- 2.13.6
On Mon, Dec 04, 2017 at 08:38:56PM -0500, John Ferlan wrote:
From: Lin Ma <lma@suse.com>
Adding an IDE controller for a machinetype that has no built-in IDE controller, libvirt will log an error. Currently the machinetype list which returns by qemuDomainMachineHasBuiltinIDE only includes 440fx, malta, sun4u and g3beige.
Signed-off-by: Lin Ma <lma@suse.com> Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml | 4 ---- ...uhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-explicit.xml | 4 ---- ...muhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml | 4 ---- .../qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml | 4 ---- ...emuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml | 4 ---- .../qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml | 4 ---- .../qemuhotplug-base-ccw-live-with-ccw-virtio.xml | 4 ---- tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live.xml | 4 ---- 8 files changed, 32 deletions(-)
ACK Jan
From: Lin Ma <lma@suse.com> Move the IDE controller check from command line building to controller def validation. Cause the IDE case for command line building to generate a failure if called to add an IDE since that shouldn't happen if the Validate code did the right thing. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 16 ---------------- src/qemu/qemu_domain.c | 27 ++++++++++++++++++++++++++- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 15d9209c6..8af30a8ca 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3120,22 +3120,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, break; case VIR_DOMAIN_CONTROLLER_TYPE_IDE: - /* Since we currently only support the integrated IDE - * controller on various boards, if we ever get to here, it's - * because some other machinetype had an IDE controller - * specified, or one with a single IDE contraller had multiple - * ide controllers specified. - */ - if (qemuDomainHasBuiltinIDE(domainDef)) - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Only a single IDE controller is supported " - "for this machine type")); - else - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("IDE controllers are unsupported for " - "this QEMU binary or machine type")); - goto error; - case VIR_DOMAIN_CONTROLLER_TYPE_FDC: case VIR_DOMAIN_CONTROLLER_TYPE_LAST: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1fc360af9..d2412154a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3959,10 +3959,32 @@ qemuDomainDeviceDefSkipController(const virDomainControllerDef *controller, static int +qemuDomainDeviceDefValidateControllerIDE(const virDomainDef *def) +{ + /* Since we currently only support the integrated IDE + * controller on various boards, if we ever get to here, it's + * because some other machinetype had an IDE controller + * specified, or one with a single IDE controller had multiple + * IDE controllers specified. + */ + if (qemuDomainHasBuiltinIDE(def)) + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Only a single IDE controller is supported " + "for this machine type")); + else + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IDE controllers are unsupported for " + "this QEMU binary or machine type")); + return -1; +} + + +static int qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, const virDomainDef *def, virQEMUCapsPtr qemuCaps) { + int ret = 0; unsigned int flags = 0; if (qemuDomainDeviceDefSkipController(controller, def, &flags)) @@ -3977,6 +3999,9 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, switch ((virDomainControllerType) controller->type) { case VIR_DOMAIN_CONTROLLER_TYPE_IDE: + ret = qemuDomainDeviceDefValidateControllerIDE(def); + break; + case VIR_DOMAIN_CONTROLLER_TYPE_FDC: case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: case VIR_DOMAIN_CONTROLLER_TYPE_SATA: @@ -3988,7 +4013,7 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, break; } - return 0; + return ret; } -- 2.13.6
Move SCSI validation from qemu_command into qemu_domain. This includes the @model reset/check when the controller model hasn't yet been set. While at it modify the switch to account for all virDomainControllerModelSCSI types rather than using the default label. Rename/reorder the args in qemuCheckSCSIControllerIOThreads to match the caller and also remove the unnecessary model check as well as fixing up the comments to remove the previously removed qemuCaps arg. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 94 +++++------------------------------------------ src/qemu/qemu_domain.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 106 insertions(+), 86 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8af30a8ca..158f73690 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2583,52 +2583,6 @@ qemuBuildUSBControllerDevStr(virDomainControllerDefPtr def, } -/* qemuCheckSCSIControllerIOThreads: - * @domainDef: Pointer to domain def - * @def: Pointer to controller def - * @qemuCaps: Capabilities - * - * If this controller definition has iothreads set, let's make sure the - * configuration is right before adding to the command line - * - * Returns true if either supported or there are no iothreads for controller; - * otherwise, returns false if configuration is not quite right. - */ -static bool -qemuCheckSCSIControllerIOThreads(const virDomainDef *domainDef, - virDomainControllerDefPtr def) -{ - if (!def->iothread) - return true; - - if (def->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("IOThreads only supported for virtio-scsi " - "controllers model is '%s'"), - virDomainControllerModelSCSITypeToString(def->model)); - return false; - } - - if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("IOThreads only available for virtio pci and " - "virtio ccw controllers")); - return false; - } - - /* Can we find the controller iothread in the iothreadid list? */ - if (!virDomainIOThreadIDFind(domainDef, def->iothread)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("controller iothread '%u' not defined in iothreadid"), - def->iothread); - return false; - } - - return true; -} - - /** * qemuBuildControllerDevStr: * @domainDef: domain definition @@ -2662,47 +2616,17 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, *devstr = NULL; - if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { - if ((qemuDomainSetSCSIControllerModel(domainDef, qemuCaps, &model)) < 0) - return -1; - } - - if (!(def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI && - model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI)) { - if (def->queues) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("'queues' is only supported by virtio-scsi controller")); - return -1; - } - if (def->cmd_per_lun) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("'cmd_per_lun' is only supported by virtio-scsi controller")); - return -1; - } - if (def->max_sectors) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("'max_sectors' is only supported by virtio-scsi controller")); - return -1; - } - if (def->ioeventfd) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("'ioeventfd' is only supported by virtio-scsi controller")); - return -1; - } - } - switch ((virDomainControllerType) def->type) { case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: - switch (model) { + if ((qemuDomainSetSCSIControllerModel(domainDef, qemuCaps, &model)) < 0) + return -1; + switch ((virDomainControllerModelSCSI) model) { case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { virBufferAddLit(&buf, "virtio-scsi-ccw"); - if (def->iothread) { - if (!qemuCheckSCSIControllerIOThreads(domainDef, def)) - goto error; + if (def->iothread) virBufferAsprintf(&buf, ",iothread=iothread%u", def->iothread); - } } else if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) { virBufferAddLit(&buf, "virtio-scsi-s390"); @@ -2711,12 +2635,9 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, virBufferAddLit(&buf, "virtio-scsi-device"); } else { virBufferAddLit(&buf, "virtio-scsi-pci"); - if (def->iothread) { - if (!qemuCheckSCSIControllerIOThreads(domainDef, def)) - goto error; + if (def->iothread) virBufferAsprintf(&buf, ",iothread=iothread%u", def->iothread); - } } if (qemuBuildVirtioOptionsStr(&buf, def->virtio, qemuCaps) < 0) goto error; @@ -2733,7 +2654,10 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078: virBufferAddLit(&buf, "megasas"); break; - default: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unsupported controller model: %s"), virDomainControllerModelSCSITypeToString(def->model)); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d2412154a..ce9904d55 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3979,6 +3979,97 @@ qemuDomainDeviceDefValidateControllerIDE(const virDomainDef *def) } +/* qemuDomainCheckSCSIControllerIOThreads: + * @controller: Pointer to controller def + * @def: Pointer to domain def + * + * If this controller definition has iothreads set, let's make sure the + * configuration is right before adding to the command line + * + * Returns true if either supported or there are no iothreads for controller; + * otherwise, returns false if configuration is not quite right. + */ +static bool +qemuDomainCheckSCSIControllerIOThreads(const virDomainControllerDef *controller, + const virDomainDef *def) +{ + if (!controller->iothread) + return true; + + if (controller->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + controller->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IOThreads only available for virtio pci and " + "virtio ccw controllers")); + return false; + } + + /* Can we find the controller iothread in the iothreadid list? */ + if (!virDomainIOThreadIDFind(def, controller->iothread)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("controller iothread '%u' not defined in iothreadid"), + controller->iothread); + return false; + } + + return true; +} + + +static int +qemuDomainDeviceDefValidateControllerSCSI(const virDomainControllerDef *controller, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + int model = controller->model; + + if ((qemuDomainSetSCSIControllerModel(def, qemuCaps, &model)) < 0) + return -1; + + if (model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI) { + if (controller->queues) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("'queues' is only supported by virtio-scsi controller")); + return -1; + } + if (controller->cmd_per_lun) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("'cmd_per_lun' is only supported by virtio-scsi controller")); + return -1; + } + if (controller->max_sectors) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("'max_sectors' is only supported by virtio-scsi controller")); + return -1; + } + if (controller->ioeventfd) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("'ioeventfd' is only supported by virtio-scsi controller")); + return -1; + } + } + + switch ((virDomainControllerModelSCSI) model) { + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: + if (!qemuDomainCheckSCSIControllerIOThreads(controller, def)) + return -1; + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST: + break; + } + + return 0; +} + + static int qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, const virDomainDef *def, @@ -4002,8 +4093,12 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, ret = qemuDomainDeviceDefValidateControllerIDE(def); break; - case VIR_DOMAIN_CONTROLLER_TYPE_FDC: case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: + ret = qemuDomainDeviceDefValidateControllerSCSI(controller, def, + qemuCaps); + break; + + case VIR_DOMAIN_CONTROLLER_TYPE_FDC: case VIR_DOMAIN_CONTROLLER_TYPE_SATA: case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: case VIR_DOMAIN_CONTROLLER_TYPE_CCID: @@ -4090,6 +4185,7 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, break; } + virObjectUnref(qemuCaps); return ret; } -- 2.13.6
Move PCI validation checks out of qemu_command into the proper qemu_domain validation helper. Since there's a lot to move, we'll start slow by replicating the check in qemuBuildControllerDevCommandLine to skip the pcie-root and pci-root tests for not pseries guests. Next we'll move the controller index check switch. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 20 -------------------- src/qemu/qemu_domain.c | 36 +++++++++++++++++++++++++++++++++++- 2 files changed, 35 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 158f73690..0089f1bf6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2719,26 +2719,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, case VIR_DOMAIN_CONTROLLER_TYPE_PCI: switch ((virDomainControllerModelPCI) def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: - case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: - if (def->idx == 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("index for pci controllers of model '%s' must be > 0"), - virDomainControllerModelPCITypeToString(def->model)); - goto error; - } - break; - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: - break; - } - switch ((virDomainControllerModelPCI) def->model) { - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: if (def->opts.pciopts.modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE || def->opts.pciopts.chassisNr == -1) { diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ce9904d55..c49a11119 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4071,6 +4071,37 @@ qemuDomainDeviceDefValidateControllerSCSI(const virDomainControllerDef *controll static int +qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controller) +{ + virDomainControllerModelPCI model = controller->model; + + switch (model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + if (controller->idx == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("index for pci controllers of model '%s' must be > 0"), + virDomainControllerModelPCITypeToString(model)); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + break; + } + + return 0; +} + + +static int qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, const virDomainDef *def, virQEMUCapsPtr qemuCaps) @@ -4098,12 +4129,15 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, qemuCaps); break; + case VIR_DOMAIN_CONTROLLER_TYPE_PCI: + ret = qemuDomainDeviceDefValidateControllerPCI(controller); + break; + case VIR_DOMAIN_CONTROLLER_TYPE_FDC: case VIR_DOMAIN_CONTROLLER_TYPE_SATA: case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: case VIR_DOMAIN_CONTROLLER_TYPE_CCID: case VIR_DOMAIN_CONTROLLER_TYPE_USB: - case VIR_DOMAIN_CONTROLLER_TYPE_PCI: case VIR_DOMAIN_CONTROLLER_TYPE_LAST: break; } -- 2.13.6
Shorten up a few characters and reference the pciopts pointer Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 115 +++++++++++++++++++++++++----------------------- 1 file changed, 59 insertions(+), 56 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0089f1bf6..94bf21803 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2612,6 +2612,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, { virBuffer buf = VIR_BUFFER_INITIALIZER; int model = def->model; + const virDomainPCIControllerOpts *pciopts; const char *modelName = NULL; *devstr = NULL; @@ -2717,24 +2718,26 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, break; case VIR_DOMAIN_CONTROLLER_TYPE_PCI: + pciopts = &def->opts.pciopts; + switch ((virDomainControllerModelPCI) def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: - if (def->opts.pciopts.modelName + if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE || - def->opts.pciopts.chassisNr == -1) { + pciopts->chassisNr == -1) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("autogenerated pci-bridge options not set")); goto error; } - modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); + modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); if (!modelName) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown pci-bridge model name value %d"), - def->opts.pciopts.modelName); + pciopts->modelName); goto error; } - if (def->opts.pciopts.modelName + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("PCI controller model name '%s' " @@ -2749,26 +2752,26 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, goto error; } virBufferAsprintf(&buf, "%s,chassis_nr=%d,id=%s", - modelName, def->opts.pciopts.chassisNr, + modelName, pciopts->chassisNr, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: - if (def->opts.pciopts.modelName + if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE || - def->opts.pciopts.busNr == -1) { + pciopts->busNr == -1) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("autogenerated pci-expander-bus options not set")); goto error; } - modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); + modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); if (!modelName) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown pci-expander-bus model name value %d"), - def->opts.pciopts.modelName); + pciopts->modelName); goto error; } - if (def->opts.pciopts.modelName + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("PCI controller model name '%s' " @@ -2783,28 +2786,28 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, goto error; } virBufferAsprintf(&buf, "%s,bus_nr=%d,id=%s", - modelName, def->opts.pciopts.busNr, + modelName, pciopts->busNr, def->info.alias); - if (def->opts.pciopts.numaNode != -1) + if (pciopts->numaNode != -1) virBufferAsprintf(&buf, ",numa_node=%d", - def->opts.pciopts.numaNode); + pciopts->numaNode); break; case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: - if (def->opts.pciopts.modelName + if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("autogenerated dmi-to-pci-bridge options not set")); goto error; } - modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); + modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); if (!modelName) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown dmi-to-pci-bridge model name value %d"), - def->opts.pciopts.modelName); + pciopts->modelName); goto error; } - if (def->opts.pciopts.modelName + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("PCI controller model name '%s' " @@ -2821,22 +2824,22 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, virBufferAsprintf(&buf, "%s,id=%s", modelName, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: - if (def->opts.pciopts.modelName + if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("autogenerated pcie-root-port options not set")); goto error; } - modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); + modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); if (!modelName) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown pcie-root-port model name value %d"), - def->opts.pciopts.modelName); + pciopts->modelName); goto error; } - if ((def->opts.pciopts.modelName != + if ((pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) && - (def->opts.pciopts.modelName != + (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("PCI controller model name '%s' " @@ -2844,7 +2847,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, modelName); goto error; } - if ((def->opts.pciopts.modelName == + if ((pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IOH3420)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -2852,7 +2855,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, "controller is not supported in this QEMU binary")); goto error; } - if ((def->opts.pciopts.modelName == + if ((pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT) && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCIE_ROOT_PORT)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -2862,24 +2865,24 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, } virBufferAsprintf(&buf, "%s,port=0x%x,chassis=%d,id=%s", - modelName, def->opts.pciopts.port, - def->opts.pciopts.chassis, def->info.alias); + modelName, pciopts->port, + pciopts->chassis, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: - if (def->opts.pciopts.modelName + if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("autogenerated pcie-switch-upstream-port options not set")); goto error; } - modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); + modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); if (!modelName) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown pcie-switch-upstream-port model name value %d"), - def->opts.pciopts.modelName); + pciopts->modelName); goto error; } - if (def->opts.pciopts.modelName + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("PCI controller model name '%s' " @@ -2897,24 +2900,24 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, virBufferAsprintf(&buf, "%s,id=%s", modelName, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: - if (def->opts.pciopts.modelName + if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE || - def->opts.pciopts.chassis == -1 || - def->opts.pciopts.port == -1) { + pciopts->chassis == -1 || + pciopts->port == -1) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("autogenerated pcie-switch-downstream-port " "options not set")); goto error; } - modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); + modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); if (!modelName) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown pcie-switch-downstream-port model name value %d"), - def->opts.pciopts.modelName); + pciopts->modelName); goto error; } - if (def->opts.pciopts.modelName + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("PCI controller model name '%s' " @@ -2930,26 +2933,26 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, goto error; } virBufferAsprintf(&buf, "%s,port=0x%x,chassis=%d,id=%s", - modelName, def->opts.pciopts.port, - def->opts.pciopts.chassis, def->info.alias); + modelName, pciopts->port, + pciopts->chassis, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: - if (def->opts.pciopts.modelName + if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE || - def->opts.pciopts.busNr == -1) { + pciopts->busNr == -1) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("autogenerated pcie-expander-bus options not set")); goto error; } - modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); + modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); if (!modelName) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown pcie-expander-bus model name value %d"), - def->opts.pciopts.modelName); + pciopts->modelName); goto error; } - if (def->opts.pciopts.modelName + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("PCI controller model name '%s' " @@ -2964,32 +2967,32 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, goto error; } virBufferAsprintf(&buf, "%s,bus_nr=%d,id=%s", - modelName, def->opts.pciopts.busNr, + modelName, pciopts->busNr, def->info.alias); - if (def->opts.pciopts.numaNode != -1) + if (pciopts->numaNode != -1) virBufferAsprintf(&buf, ",numa_node=%d", - def->opts.pciopts.numaNode); + pciopts->numaNode); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: - if (def->opts.pciopts.modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE || - def->opts.pciopts.targetIndex == -1) { + if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE || + pciopts->targetIndex == -1) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("autogenerated pci-root options not set")); goto error; } /* Skip the implicit one */ - if (def->opts.pciopts.targetIndex == 0) + if (pciopts->targetIndex == 0) goto done; - modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); + modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); if (!modelName) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown pci-root model name value %d"), - def->opts.pciopts.modelName); + pciopts->modelName); goto error; } - if (def->opts.pciopts.modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) { + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("PCI controller model name '%s' is not valid for a pci-root"), modelName); @@ -3002,17 +3005,17 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, goto error; } virBufferAsprintf(&buf, "%s,index=%d,id=%s", - modelName, def->opts.pciopts.targetIndex, + modelName, pciopts->targetIndex, def->info.alias); - if (def->opts.pciopts.numaNode != -1) { + if (pciopts->numaNode != -1) { if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("the spapr-pci-host-bridge controller " "doesn't support numa_node on this QEMU binary")); goto error; } - virBufferAsprintf(&buf, ",numa_node=%d", def->opts.pciopts.numaNode); + virBufferAsprintf(&buf, ",numa_node=%d", pciopts->numaNode); } break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: -- 2.13.6
Move the various modelName == NAME_NONE from the command line generation into domain controller validation. Also rather than have multiple cases with the same check, let's make the code more generic, but also note that it was the modelName option that caused the failure. We also have to be sure not to check the PCI models that we don't care about. For the remaining checks in command line building, we can use the field name in the error message to be more specific about what causes the failure. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 52 ++++++++++++------------------------------------- src/qemu/qemu_domain.c | 12 ++++++++++++ 2 files changed, 24 insertions(+), 40 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 94bf21803..9f8a1bc33 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2722,11 +2722,9 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, switch ((virDomainControllerModelPCI) def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: - if (pciopts->modelName - == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE || - pciopts->chassisNr == -1) { + if (pciopts->chassisNr == -1) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pci-bridge options not set")); + _("autogenerated pci-bridge option chassisNr not set")); goto error; } @@ -2756,12 +2754,9 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: - if (pciopts->modelName - == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE || - pciopts->busNr == -1) { + if (pciopts->busNr == -1) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pci-expander-bus options not set")); - goto error; + _("autogenerated pci-expander-bus option busNr not set")); } modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); @@ -2793,13 +2788,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, pciopts->numaNode); break; case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: - if (pciopts->modelName - == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated dmi-to-pci-bridge options not set")); - goto error; - } - modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); if (!modelName) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2824,12 +2812,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, virBufferAsprintf(&buf, "%s,id=%s", modelName, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: - if (pciopts->modelName - == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pcie-root-port options not set")); - goto error; - } modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); if (!modelName) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2869,12 +2851,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, pciopts->chassis, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: - if (pciopts->modelName - == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pcie-switch-upstream-port options not set")); - goto error; - } modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); if (!modelName) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2900,13 +2876,12 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, virBufferAsprintf(&buf, "%s,id=%s", modelName, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: - if (pciopts->modelName - == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE || - pciopts->chassis == -1 || + if (pciopts->chassis == -1 || pciopts->port == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + virReportError(VIR_ERR_INTERNAL_ERROR, _("autogenerated pcie-switch-downstream-port " - "options not set")); + "options chassis=%d or port=%d not set"), + pciopts->chassis, pciopts->port); goto error; } @@ -2937,11 +2912,9 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, pciopts->chassis, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: - if (pciopts->modelName - == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE || - pciopts->busNr == -1) { + if (pciopts->busNr == -1) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pcie-expander-bus options not set")); + _("autogenerated pcie-expander-bus option busNr not set")); goto error; } @@ -2974,10 +2947,9 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, pciopts->numaNode); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: - if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE || - pciopts->targetIndex == -1) { + if (pciopts->targetIndex == -1) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pci-root options not set")); + _("autogenerated pci-root option targetIndex not set")); goto error; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c49a11119..eb7941f10 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4074,6 +4074,7 @@ static int qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controller) { virDomainControllerModelPCI model = controller->model; + const virDomainPCIControllerOpts *pciopts; switch (model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: @@ -4097,6 +4098,17 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle break; } + pciopts = &controller->opts.pciopts; + if (controller->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT && + controller->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST) { + if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("autogenerated %s option modelName not set"), + virDomainControllerModelPCITypeToString(controller->model)); + return -1; + } + } + return 0; } -- 2.13.6
Similar to the checking the modelName vs. NAME_NONE, let's make the ModelNameTypeToString check more generic too within the checking done in controller validation (with the same ignore certain models. NB: We need to keep the ModelNameTypeToString fetch in command line validation since we use it, but at least we can assume it returns something valid now. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 59 +++---------------------------------------------- src/qemu/qemu_domain.c | 10 +++++++++ 2 files changed, 13 insertions(+), 56 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9f8a1bc33..7a9b68604 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2719,6 +2719,9 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, case VIR_DOMAIN_CONTROLLER_TYPE_PCI: pciopts = &def->opts.pciopts; + if (def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT && + def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST) + modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); switch ((virDomainControllerModelPCI) def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: @@ -2728,13 +2731,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, goto error; } - modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); - if (!modelName) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown pci-bridge model name value %d"), - pciopts->modelName); - goto error; - } if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -2759,13 +2755,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, _("autogenerated pci-expander-bus option busNr not set")); } - modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); - if (!modelName) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown pci-expander-bus model name value %d"), - pciopts->modelName); - goto error; - } if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -2788,13 +2777,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, pciopts->numaNode); break; case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: - modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); - if (!modelName) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown dmi-to-pci-bridge model name value %d"), - pciopts->modelName); - goto error; - } if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -2812,13 +2794,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, virBufferAsprintf(&buf, "%s,id=%s", modelName, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: - modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); - if (!modelName) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown pcie-root-port model name value %d"), - pciopts->modelName); - goto error; - } if ((pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) && (pciopts->modelName != @@ -2851,13 +2826,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, pciopts->chassis, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: - modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); - if (!modelName) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown pcie-switch-upstream-port model name value %d"), - pciopts->modelName); - goto error; - } if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -2885,13 +2853,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, goto error; } - modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); - if (!modelName) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown pcie-switch-downstream-port model name value %d"), - pciopts->modelName); - goto error; - } if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -2918,13 +2879,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, goto error; } - modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); - if (!modelName) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown pcie-expander-bus model name value %d"), - pciopts->modelName); - goto error; - } if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -2957,13 +2911,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, if (pciopts->targetIndex == 0) goto done; - modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); - if (!modelName) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown pci-root model name value %d"), - pciopts->modelName); - goto error; - } if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("PCI controller model name '%s' is not valid for a pci-root"), diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index eb7941f10..65487880e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4075,6 +4075,7 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle { virDomainControllerModelPCI model = controller->model; const virDomainPCIControllerOpts *pciopts; + const char *modelName = NULL; switch (model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: @@ -4107,6 +4108,15 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle virDomainControllerModelPCITypeToString(controller->model)); return -1; } + + modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); + if (!modelName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown %s modelName value %d"), + virDomainControllerModelPCITypeToString(controller->model), + pciopts->modelName); + return -1; + } } return 0; -- 2.13.6
Excluding the qemuCaps checks, move the remainder of the checks that validate whether the PCI definition is valid or not into qemuDomainControllerDefValidatePCI. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 96 ---------------------------------- src/qemu/qemu_domain.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 96 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7a9b68604..e722bb46b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2725,20 +2725,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, switch ((virDomainControllerModelPCI) def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: - if (pciopts->chassisNr == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pci-bridge option chassisNr not set")); - goto error; - } - - if (pciopts->modelName - != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller model name '%s' " - "is not valid for a pci-bridge"), - modelName); - goto error; - } if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("the pci-bridge controller " @@ -2750,19 +2736,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: - if (pciopts->busNr == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pci-expander-bus option busNr not set")); - } - - if (pciopts->modelName - != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller model name '%s' " - "is not valid for a pci-expander-bus"), - modelName); - goto error; - } if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("the pxb controller " @@ -2777,14 +2750,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, pciopts->numaNode); break; case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: - if (pciopts->modelName - != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller model name '%s' " - "is not valid for a dmi-to-pci-bridge"), - modelName); - goto error; - } if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("the dmi-to-pci-bridge (i82801b11-bridge) " @@ -2794,16 +2759,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, virBufferAsprintf(&buf, "%s,id=%s", modelName, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: - if ((pciopts->modelName != - VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) && - (pciopts->modelName != - VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller model name '%s' " - "is not valid for a pcie-root-port"), - modelName); - goto error; - } if ((pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IOH3420)) { @@ -2826,14 +2781,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, pciopts->chassis, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: - if (pciopts->modelName - != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller model name '%s' " - "is not valid for a pcie-switch-upstream-port"), - modelName); - goto error; - } if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_X3130_UPSTREAM)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("the pcie-switch-upstream-port (x3130-upstream) " @@ -2844,23 +2791,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, virBufferAsprintf(&buf, "%s,id=%s", modelName, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: - if (pciopts->chassis == -1 || - pciopts->port == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("autogenerated pcie-switch-downstream-port " - "options chassis=%d or port=%d not set"), - pciopts->chassis, pciopts->port); - goto error; - } - - if (pciopts->modelName - != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller model name '%s' " - "is not valid for a pcie-switch-downstream-port"), - modelName); - goto error; - } if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("The pcie-switch-downstream-port " @@ -2873,20 +2803,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, pciopts->chassis, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: - if (pciopts->busNr == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pcie-expander-bus option busNr not set")); - goto error; - } - - if (pciopts->modelName - != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller model name '%s' " - "is not valid for a pcie-expander-bus"), - modelName); - goto error; - } if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB_PCIE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("the pxb-pcie controller " @@ -2901,22 +2817,10 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, pciopts->numaNode); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: - if (pciopts->targetIndex == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pci-root option targetIndex not set")); - goto error; - } - /* Skip the implicit one */ if (pciopts->targetIndex == 0) goto done; - if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller model name '%s' is not valid for a pci-root"), - modelName); - goto error; - } if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("the spapr-pci-host-bridge controller " diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 65487880e..86e6d5573 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4077,6 +4077,8 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle const virDomainPCIControllerOpts *pciopts; const char *modelName = NULL; + /* First pass - just check the controller index for the model's + * that we care to check... */ switch (model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: @@ -4119,6 +4121,138 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle } } + /* Second pass - now the model specific checks */ + switch (model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + if (pciopts->chassisNr == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated pci-bridge option chassisNr not set")); + return -1; + } + + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller model name '%s' is not valid " + "for a pci-bridge"), + modelName); + return -1; + } + + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + if (pciopts->busNr == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated pci-expander-bus option busNr not set")); + return -1; + } + + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller model name '%s' is not valid " + "for a pci-expander-bus"), + modelName); + return -1; + } + + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller model name '%s' is not valid " + "for a dmi-to-pci-bridge"), + modelName); + return -1; + } + + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + if ((pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) && + (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller model name '%s' is not valid " + "for a pcie-root-port"), + modelName); + return -1; + } + + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller model name '%s' is not valid " + "for a pcie-switch-upstream-port"), + modelName); + return -1; + } + + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + if (pciopts->chassis == -1 || pciopts->port == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("autogenerated pcie-switch-downstream-port " + "options chassis=%d or port=%d not set"), + pciopts->chassis, pciopts->port); + return -1; + } + + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller model name '%s' is not valid " + "for a pcie-switch-downstream-port"), + modelName); + return -1; + } + + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + if (pciopts->busNr == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated pcie-expander-bus option busNr not set")); + return -1; + } + + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller model name '%s' is not valid " + "for a pcie-expander-bus"), + modelName); + return -1; + } + + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + if (pciopts->targetIndex == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated pci-root option targetIndex not set")); + return -1; + } + + /* Skip the implicit one */ + if (pciopts->targetIndex == 0) + return 0; + + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller model name '%s' is not valid " + "for a pci-root"), + modelName); + return 0; + } + + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + break; + } + return 0; } -- 2.13.6
Move the qemuCaps checks over to qemuDomainControllerDefValidatePCI and add empty lines between cases so it's a bit easier on the eyes to read. This requires two test updates in order to set the correct capability bit for an xml2xml test as well as setting up the similar capability for the pseries memlocktest. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 78 ++++++------------------------------------------ src/qemu/qemu_domain.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++-- tests/qemumemlocktest.c | 14 +++++++++ tests/qemuxml2xmltest.c | 5 +++- 4 files changed, 104 insertions(+), 72 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e722bb46b..837339d23 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2725,23 +2725,12 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, switch ((virDomainControllerModelPCI) def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the pci-bridge controller " - "is not supported in this QEMU binary")); - goto error; - } virBufferAsprintf(&buf, "%s,chassis_nr=%d,id=%s", modelName, pciopts->chassisNr, def->info.alias); break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the pxb controller " - "is not supported in this QEMU binary")); - goto error; - } virBufferAsprintf(&buf, "%s,bus_nr=%d,id=%s", modelName, pciopts->busNr, def->info.alias); @@ -2749,66 +2738,28 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, virBufferAsprintf(&buf, ",numa_node=%d", pciopts->numaNode); break; + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the dmi-to-pci-bridge (i82801b11-bridge) " - "controller is not supported in this QEMU binary")); - goto error; - } virBufferAsprintf(&buf, "%s,id=%s", modelName, def->info.alias); break; - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: - if ((pciopts->modelName == - VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IOH3420)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the pcie-root-port (ioh3420) " - "controller is not supported in this QEMU binary")); - goto error; - } - if ((pciopts->modelName == - VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT) && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCIE_ROOT_PORT)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the pcie-root-port (pcie-root-port) " - "controller is not supported in this QEMU binary")); - goto error; - } + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: virBufferAsprintf(&buf, "%s,port=0x%x,chassis=%d,id=%s", modelName, pciopts->port, pciopts->chassis, def->info.alias); break; - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_X3130_UPSTREAM)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the pcie-switch-upstream-port (x3130-upstream) " - "controller is not supported in this QEMU binary")); - goto error; - } + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: virBufferAsprintf(&buf, "%s,id=%s", modelName, def->info.alias); break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("The pcie-switch-downstream-port " - "(xio3130-downstream) controller " - "is not supported in this QEMU binary")); - goto error; - } virBufferAsprintf(&buf, "%s,port=0x%x,chassis=%d,id=%s", modelName, pciopts->port, pciopts->chassis, def->info.alias); break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB_PCIE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the pxb-pcie controller " - "is not supported in this QEMU binary")); - goto error; - } virBufferAsprintf(&buf, "%s,bus_nr=%d,id=%s", modelName, pciopts->busNr, def->info.alias); @@ -2816,31 +2767,20 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, virBufferAsprintf(&buf, ",numa_node=%d", pciopts->numaNode); break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: /* Skip the implicit one */ if (pciopts->targetIndex == 0) goto done; - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the spapr-pci-host-bridge controller " - "is not supported in this QEMU binary")); - goto error; - } virBufferAsprintf(&buf, "%s,index=%d,id=%s", modelName, pciopts->targetIndex, def->info.alias); - if (pciopts->numaNode != -1) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the spapr-pci-host-bridge controller " - "doesn't support numa_node on this QEMU binary")); - goto error; - } + if (pciopts->numaNode != -1) virBufferAsprintf(&buf, ",numa_node=%d", pciopts->numaNode); - } break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 86e6d5573..0aed767d2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4071,7 +4071,8 @@ qemuDomainDeviceDefValidateControllerSCSI(const virDomainControllerDef *controll static int -qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controller) +qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controller, + virQEMUCapsPtr qemuCaps) { virDomainControllerModelPCI model = controller->model; const virDomainPCIControllerOpts *pciopts; @@ -4138,6 +4139,13 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle return -1; } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the pci-bridge controller is not supported " + "in this QEMU binary")); + return -1; + } + break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: @@ -4155,6 +4163,13 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle return -1; } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the pxb controller is not supported in this " + "QEMU binary")); + return -1; + } + break; case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: @@ -4166,6 +4181,13 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle return -1; } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the dmi-to-pci-bridge (i82801b11-bridge) " + "controller is not supported in this QEMU binary")); + return -1; + } + break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: @@ -4178,6 +4200,22 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle return -1; } + if ((pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IOH3420)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the pcie-root-port (ioh3420) controller " + "is not supported in this QEMU binary")); + return -1; + } + + if ((pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCIE_ROOT_PORT)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the pcie-root-port (pcie-root-port) controller " + "is not supported in this QEMU binary")); + return -1; + } + break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: @@ -4189,6 +4227,13 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle return -1; } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_X3130_UPSTREAM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the pcie-switch-upstream-port (x3130-upstream) " + "controller is not supported in this QEMU binary")); + return -1; + } + break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: @@ -4208,6 +4253,14 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle return -1; } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("The pcie-switch-downstream-port " + "(xio3130-downstream) controller is not " + "supported in this QEMU binary")); + return -1; + } + break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: @@ -4225,6 +4278,13 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle return -1; } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB_PCIE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the pxb-pcie controller is not supported " + "in this QEMU binary")); + return -1; + } + break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: @@ -4246,6 +4306,21 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle return 0; } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the spapr-pci-host-bridge controller is not " + "supported in this QEMU binary")); + return -1; + } + + if (pciopts->numaNode != -1 && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the spapr-pci-host-bridge controller doesn't " + "support numa_node in this QEMU binary")); + return -1; + } + break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: @@ -4286,7 +4361,7 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, break; case VIR_DOMAIN_CONTROLLER_TYPE_PCI: - ret = qemuDomainDeviceDefValidateControllerPCI(controller); + ret = qemuDomainDeviceDefValidateControllerPCI(controller, qemuCaps); break; case VIR_DOMAIN_CONTROLLER_TYPE_FDC: diff --git a/tests/qemumemlocktest.c b/tests/qemumemlocktest.c index 62bd25450..bc0b53e6f 100644 --- a/tests/qemumemlocktest.c +++ b/tests/qemumemlocktest.c @@ -63,6 +63,7 @@ mymain(void) { int ret = 0; char *fakerootdir; + virQEMUCapsPtr qemuCaps = NULL; if (VIR_STRDUP_QUIET(fakerootdir, FAKEROOTDIRTEMPLATE) < 0) { fprintf(stderr, "Out of memory\n"); @@ -127,6 +128,16 @@ mymain(void) DO_TEST("pc-locked+hostdev", VIR_DOMAIN_MEMORY_PARAM_UNLIMITED); qemuTestSetHostArch(driver.caps, VIR_ARCH_PPC64); + if (!(qemuCaps = virQEMUCapsNew())) { + ret = -1; + goto cleanup; + } + + virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE); + if (qemuTestCapsCacheInsert(driver.qemuCapsCache, qemuCaps) < 0) { + ret = -1; + goto cleanup; + }; DO_TEST("pseries-kvm", 20971520); DO_TEST("pseries-tcg", 0); @@ -140,6 +151,9 @@ mymain(void) DO_TEST("pseries-hardlimit+locked+hostdev", 2147483648); DO_TEST("pseries-locked+hostdev", VIR_DOMAIN_MEMORY_PARAM_UNLIMITED); + cleanup: + virObjectUnref(qemuCaps); + if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) virFileDeleteTree(fakerootdir); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 146a67ee2..790d976f4 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1310,7 +1310,10 @@ mymain(void) DO_TEST("intel-iommu-machine", QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_MACHINE_IOMMU); - DO_TEST("intel-iommu-caching-mode", NONE); + DO_TEST("intel-iommu-caching-mode", + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420); DO_TEST("intel-iommu-eim", NONE); DO_TEST("intel-iommu-device-iotlb", NONE); -- 2.13.6
Move the SATA controller check from command line building to controller def validation. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 6 ------ src/qemu/qemu_domain.c | 17 ++++++++++++++++- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 837339d23..56030c482 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2699,12 +2699,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, break; case VIR_DOMAIN_CONTROLLER_TYPE_SATA: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_AHCI)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("SATA is not supported with this " - "QEMU binary")); - goto error; - } virBufferAsprintf(&buf, "ahci,id=%s", def->info.alias); break; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0aed767d2..2e82df4ff 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4333,6 +4333,18 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle static int +qemuDomainDeviceDefValidateControllerSATA(virQEMUCapsPtr qemuCaps) +{ + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_AHCI)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("SATA is not supported with this QEMU binary")); + return -1; + } + return 0; +} + + +static int qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, const virDomainDef *def, virQEMUCapsPtr qemuCaps) @@ -4364,8 +4376,11 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, ret = qemuDomainDeviceDefValidateControllerPCI(controller, qemuCaps); break; - case VIR_DOMAIN_CONTROLLER_TYPE_FDC: case VIR_DOMAIN_CONTROLLER_TYPE_SATA: + ret = qemuDomainDeviceDefValidateControllerSATA(qemuCaps); + break; + + case VIR_DOMAIN_CONTROLLER_TYPE_FDC: case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: case VIR_DOMAIN_CONTROLLER_TYPE_CCID: case VIR_DOMAIN_CONTROLLER_TYPE_USB: -- 2.13.6
Move the USB controller validation checs out of qemu_command and into the proper qemu_domain validation helper. We will start slowly and also modity the xml2argv test to change from a RUN to PARSE failure check when the QEMU_CAPS_PIIX3_USB_UHCI is removed. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 6 ------ src/qemu/qemu_domain.c | 18 +++++++++++++++++- tests/qemuxml2argvtest.c | 8 ++++---- 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 56030c482..7ea01e008 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2542,12 +2542,6 @@ qemuBuildUSBControllerDevStr(virDomainControllerDefPtr def, model = def->model; - if (model == -1) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - "%s", _("no model provided for USB controller")); - return -1; - } - smodel = qemuControllerModelUSBTypeToString(model); flags = qemuControllerModelUSBToCaps(model); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2e82df4ff..3ab3aa181 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4345,6 +4345,19 @@ qemuDomainDeviceDefValidateControllerSATA(virQEMUCapsPtr qemuCaps) static int +qemuDomainDeviceDefValidateControllerUSB(const virDomainControllerDef *controller) +{ + if (controller->model == -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("no model provided for USB controller")); + return -1; + } + + return 0; +} + + +static int qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, const virDomainDef *def, virQEMUCapsPtr qemuCaps) @@ -4380,10 +4393,13 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, ret = qemuDomainDeviceDefValidateControllerSATA(qemuCaps); break; + case VIR_DOMAIN_CONTROLLER_TYPE_USB: + ret = qemuDomainDeviceDefValidateControllerUSB(controller); + break; + case VIR_DOMAIN_CONTROLLER_TYPE_FDC: case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: case VIR_DOMAIN_CONTROLLER_TYPE_CCID: - case VIR_DOMAIN_CONTROLLER_TYPE_USB: case VIR_DOMAIN_CONTROLLER_TYPE_LAST: break; } diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 385a54615..3fcc76baa 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1508,10 +1508,10 @@ mymain(void) QEMU_CAPS_PCI_OHCI, QEMU_CAPS_PIIX3_USB_UHCI, QEMU_CAPS_NEC_USB_XHCI); - DO_TEST_FAILURE("usb-controller-default-unavailable-q35", - QEMU_CAPS_DEVICE_IOH3420, - QEMU_CAPS_PCI_OHCI, - QEMU_CAPS_NEC_USB_XHCI); + DO_TEST_PARSE_ERROR("usb-controller-default-unavailable-q35", + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_PCI_OHCI, + QEMU_CAPS_NEC_USB_XHCI); DO_TEST("usb-controller-explicit-q35", QEMU_CAPS_DEVICE_IOH3420, QEMU_CAPS_PCI_OHCI, -- 2.13.6
On Mon, Dec 04, 2017 at 08:39:06PM -0500, John Ferlan wrote:
Move the USB controller validation checs out of qemu_command and
*checks
into the proper qemu_domain validation helper.
We will start slowly and also modity the xml2argv test to change
*modify
from a RUN to PARSE failure check when the QEMU_CAPS_PIIX3_USB_UHCI is removed.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 6 ------ src/qemu/qemu_domain.c | 18 +++++++++++++++++- tests/qemuxml2argvtest.c | 8 ++++---- 3 files changed, 21 insertions(+), 11 deletions(-)
Jan
Move the remainder of the qemuBuildUSBControllerDevStr checks over to qemuDomainDeviceDefValidateControllerUSB. This also allows the command code to shorten up a bit and become a void procedure. This also requires modifying the xml2xml test a bit in order to include the correct capability bit and modifying the xml2argv test in order to use the PARSE_ERROR macro. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 64 +++--------------------------------------------- src/qemu/qemu_command.h | 2 ++ src/qemu/qemu_domain.c | 55 +++++++++++++++++++++++++++++++++++++++-- tests/qemuxml2argvtest.c | 10 ++++---- tests/qemuxml2xmltest.c | 17 ++++++++----- 5 files changed, 74 insertions(+), 74 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7ea01e008..0d4355abc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -142,8 +142,6 @@ VIR_ENUM_IMPL(qemuSoundCodec, VIR_DOMAIN_SOUND_CODEC_TYPE_LAST, "hda-duplex", "hda-micro"); -VIR_ENUM_DECL(qemuControllerModelUSB) - VIR_ENUM_IMPL(qemuControllerModelUSB, VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST, "piix3-usb-uhci", "piix4-usb-uhci", @@ -2503,66 +2501,13 @@ qemuBuildFSDevCommandLine(virCommandPtr cmd, } -static int -qemuControllerModelUSBToCaps(int model) -{ - switch (model) { - case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI: - return QEMU_CAPS_PIIX3_USB_UHCI; - case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX4_UHCI: - return QEMU_CAPS_PIIX4_USB_UHCI; - case VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI: - return QEMU_CAPS_USB_EHCI; - case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1: - case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1: - case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2: - case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3: - return QEMU_CAPS_ICH9_USB_EHCI1; - case VIR_DOMAIN_CONTROLLER_MODEL_USB_VT82C686B_UHCI: - return QEMU_CAPS_VT82C686B_USB_UHCI; - case VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI: - return QEMU_CAPS_PCI_OHCI; - case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI: - return QEMU_CAPS_NEC_USB_XHCI; - case VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI: - return QEMU_CAPS_DEVICE_QEMU_XHCI; - default: - return -1; - } -} - - -static int +static void qemuBuildUSBControllerDevStr(virDomainControllerDefPtr def, - virQEMUCapsPtr qemuCaps, virBuffer *buf) { - const char *smodel; - int model, flags; - - model = def->model; - - smodel = qemuControllerModelUSBTypeToString(model); - flags = qemuControllerModelUSBToCaps(model); - - if (flags == -1 || !virQEMUCapsGet(qemuCaps, flags)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("%s not supported in this QEMU binary"), smodel); - return -1; - } - - virBufferAsprintf(buf, "%s", smodel); + virBufferAsprintf(buf, "%s", qemuControllerModelUSBTypeToString(def->model)); if (def->opts.usbopts.ports != -1) { - if ((model != VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI || - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI_PORTS)) && - model != VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("usb controller type %s doesn't support 'ports' " - "with this QEMU binary"), smodel); - return -1; - } - virBufferAsprintf(buf, ",p2=%d,p3=%d", def->opts.usbopts.ports, def->opts.usbopts.ports); } @@ -2572,8 +2517,6 @@ qemuBuildUSBControllerDevStr(virDomainControllerDefPtr def, def->info.alias, def->info.master.usb.startport); else virBufferAsprintf(buf, ",id=%s", def->info.alias); - - return 0; } @@ -2697,8 +2640,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, break; case VIR_DOMAIN_CONTROLLER_TYPE_USB: - if (qemuBuildUSBControllerDevStr(def, qemuCaps, &buf) == -1) - goto error; + qemuBuildUSBControllerDevStr(def, &buf); if (nusbcontroller) *nusbcontroller += 1; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index bdde6f918..3551b8cfb 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -43,6 +43,8 @@ VIR_ENUM_DECL(qemuVideo) +VIR_ENUM_DECL(qemuControllerModelUSB) + virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver, virLogManagerPtr logManager, virDomainObjPtr vm, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3ab3aa181..118216d84 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4344,15 +4344,66 @@ qemuDomainDeviceDefValidateControllerSATA(virQEMUCapsPtr qemuCaps) } +static unsigned int +qemuDomainControllerModelUSBToCaps(int model) +{ + switch (model) { + case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI: + return QEMU_CAPS_PIIX3_USB_UHCI; + case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX4_UHCI: + return QEMU_CAPS_PIIX4_USB_UHCI; + case VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI: + return QEMU_CAPS_USB_EHCI; + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3: + return QEMU_CAPS_ICH9_USB_EHCI1; + case VIR_DOMAIN_CONTROLLER_MODEL_USB_VT82C686B_UHCI: + return QEMU_CAPS_VT82C686B_USB_UHCI; + case VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI: + return QEMU_CAPS_PCI_OHCI; + case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI: + return QEMU_CAPS_NEC_USB_XHCI; + case VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI: + return QEMU_CAPS_DEVICE_QEMU_XHCI; + default: + return -1; + } +} + + static int -qemuDomainDeviceDefValidateControllerUSB(const virDomainControllerDef *controller) +qemuDomainDeviceDefValidateControllerUSB(const virDomainControllerDef *controller, + virQEMUCapsPtr qemuCaps) { + unsigned int flags; + if (controller->model == -1) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("no model provided for USB controller")); return -1; } + flags = qemuDomainControllerModelUSBToCaps(controller->model); + if (flags == -1 || !virQEMUCapsGet(qemuCaps, flags)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("%s not supported in this QEMU binary"), + qemuControllerModelUSBTypeToString(controller->model)); + return -1; + } + + if (controller->opts.usbopts.ports != -1 && + (controller->model != VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI || + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI_PORTS)) && + controller->model != VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("usb controller type %s doesn't support 'ports' " + "with this QEMU binary"), + qemuControllerModelUSBTypeToString(controller->model)); + return -1; + } + return 0; } @@ -4394,7 +4445,7 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, break; case VIR_DOMAIN_CONTROLLER_TYPE_USB: - ret = qemuDomainDeviceDefValidateControllerUSB(controller); + ret = qemuDomainDeviceDefValidateControllerUSB(controller, qemuCaps); break; case VIR_DOMAIN_CONTROLLER_TYPE_FDC: diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 3fcc76baa..439d570d8 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1517,10 +1517,10 @@ mymain(void) QEMU_CAPS_PCI_OHCI, QEMU_CAPS_PIIX3_USB_UHCI, QEMU_CAPS_NEC_USB_XHCI); - DO_TEST_FAILURE("usb-controller-explicit-unavailable-q35", - QEMU_CAPS_DEVICE_IOH3420, - QEMU_CAPS_PCI_OHCI, - QEMU_CAPS_PIIX3_USB_UHCI); + DO_TEST_PARSE_ERROR("usb-controller-explicit-unavailable-q35", + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_PCI_OHCI, + QEMU_CAPS_PIIX3_USB_UHCI); DO_TEST("usb-controller-xhci", QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_PIIX3_USB_UHCI, @@ -1538,7 +1538,7 @@ mymain(void) QEMU_CAPS_NEC_USB_XHCI, QEMU_CAPS_NEC_USB_XHCI_PORTS); DO_TEST("usb-controller-qemu-xhci", QEMU_CAPS_DEVICE_QEMU_XHCI); - DO_TEST_FAILURE("usb-controller-qemu-xhci-unavailable", NONE); + DO_TEST_PARSE_ERROR("usb-controller-qemu-xhci-unavailable", NONE); DO_TEST_PARSE_ERROR("usb-controller-qemu-xhci-limit", QEMU_CAPS_DEVICE_QEMU_XHCI); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 790d976f4..d4f599a99 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -678,15 +678,18 @@ mymain(void) DO_TEST("ppc64-usb-controller-legacy", QEMU_CAPS_PIIX3_USB_UHCI); DO_TEST("usb-port-missing", NONE); - DO_TEST("usb-redir", NONE); - DO_TEST("usb-redir-filter", NONE); + DO_TEST("usb-redir", + QEMU_CAPS_ICH9_USB_EHCI1); + DO_TEST("usb-redir-filter", + QEMU_CAPS_ICH9_USB_EHCI1); DO_TEST("usb-redir-filter-version", NONE); DO_TEST("blkdeviotune", NONE); DO_TEST("blkdeviotune-max", NONE); DO_TEST("blkdeviotune-group-num", NONE); DO_TEST("blkdeviotune-max-length", NONE); - DO_TEST("controller-usb-order", NONE); - + DO_TEST("controller-usb-order", + QEMU_CAPS_NEC_USB_XHCI, + QEMU_CAPS_ICH9_USB_EHCI1); DO_TEST_FULL("seclabel-dynamic-baselabel", WHEN_INACTIVE, GIC_NONE, NONE); DO_TEST_FULL("seclabel-dynamic-override", WHEN_INACTIVE, GIC_NONE, NONE); DO_TEST_FULL("seclabel-dynamic-labelskip", WHEN_INACTIVE, GIC_NONE, NONE); @@ -829,7 +832,8 @@ mymain(void) DO_TEST("numad-auto-vcpu-no-numatune", NONE); DO_TEST("numad-auto-memory-vcpu-no-cpuset-and-placement", NONE); DO_TEST("numad-auto-memory-vcpu-cpuset", NONE); - DO_TEST("usb-ich9-ehci-addr", NONE); + DO_TEST("usb-ich9-ehci-addr", + QEMU_CAPS_ICH9_USB_EHCI1); DO_TEST("disk-copy_on_read", NONE); DO_TEST("tpm-passthrough", NONE); @@ -1313,7 +1317,8 @@ mymain(void) DO_TEST("intel-iommu-caching-mode", QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, - QEMU_CAPS_DEVICE_IOH3420); + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_ICH9_USB_EHCI1); DO_TEST("intel-iommu-eim", NONE); DO_TEST("intel-iommu-device-iotlb", NONE); -- 2.13.6
participants (2)
-
John Ferlan -
Ján Tomko