[libvirt] [PATCH v3 00/12] Move qemu command line controller checks to qemuDomainDeviceDefValidateController* checks
v2: https://www.redhat.com/archives/libvir-list/2017-December/msg00173.html Changes since v2: * Push a few patches from v1 that were ACK'd * Make the Skip helper private to qemu_command.c and just copied the various skip checks from the new helper into the specific controller validate helper to make it painfully obvious what was being skipped rather than having common code handle that and possibly obsfuscate those details from the casual reader. John Ferlan (11): qemu: Introduce qemuBuildSkipController qemu: Introduce qemuDomainDeviceDefValidateController 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 Lin Ma (1): qemu: Introduce qemuDomainDeviceDefValidateControllerIDE src/qemu/qemu_command.c | 497 ++++++++---------------------------------------- src/qemu/qemu_domain.c | 469 ++++++++++++++++++++++++++++++++++++++++++++- tests/qemumemlocktest.c | 14 ++ tests/qemuxml2xmltest.c | 5 +- 4 files changed, 564 insertions(+), 421 deletions(-) -- 2.13.6
Move the non USB implicit controller checks into their own helper to reduce the cruft in qemuBuildControllerDevCommandLine. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 65 +++++++++++++++++++++++++++++++------------------ 1 file changed, 41 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 645802795..ffa89aa4a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3175,6 +3175,44 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, } +/** + * qemuBuildSkipController: + * @controller: Controller to check + * @def: Domain definition + * + * Returns true if this controller can be skipped for command line + * generation or device validation. + */ +static bool +qemuBuildSkipController(const virDomainControllerDef *controller, + const virDomainDef *def) +{ + /* 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; + + return false; +} + + static int qemuBuildControllerDevCommandLine(virCommandPtr cmd, const virDomainDef *def, @@ -3217,6 +3255,9 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, if (cont->type != contOrder[j]) continue; + if (qemuBuildSkipController(cont, def)) + continue; + /* skip USB controllers with type none.*/ if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) { @@ -3224,30 +3265,6 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, continue; } - /* skip pcie-root */ - if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && - cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) { - continue; - } - - /* 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) && -- 2.13.6
On Wed, Dec 06, 2017 at 08:13:57PM -0500, John Ferlan wrote:
Move the non USB implicit controller checks into their own helper to reduce the cruft in qemuBuildControllerDevCommandLine.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 65 +++++++++++++++++++++++++++++++------------------ 1 file changed, 41 insertions(+), 24 deletions(-)
ACK Jan
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 | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 75cecb1bb..877a0a322 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3893,6 +3893,26 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk) static int +qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller) +{ + 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) @@ -3936,11 +3956,14 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, ret = qemuDomainDeviceDefValidateDisk(dev->data.disk); break; + case VIR_DOMAIN_DEVICE_CONTROLLER: + ret = qemuDomainDeviceDefValidateController(dev->data.controller); + 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
On Wed, Dec 06, 2017 at 08:13:58PM -0500, John Ferlan wrote:
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 | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)
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 | 20 +++++++++++++++++--- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ffa89aa4a..894c3f323 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 877a0a322..8d966156d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3893,8 +3893,14 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk) static int -qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller) +qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) { + 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: @@ -3915,9 +3921,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: @@ -3957,7 +3969,8 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, break; case VIR_DOMAIN_DEVICE_CONTROLLER: - ret = qemuDomainDeviceDefValidateController(dev->data.controller); + ret = qemuDomainDeviceDefValidateController(dev->data.controller, def, + qemuCaps); break; case VIR_DOMAIN_DEVICE_LEASE: @@ -3978,6 +3991,7 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, break; } + virObjectUnref(qemuCaps); return ret; } -- 2.13.6
On Wed, Dec 06, 2017 at 08:13:59PM -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 | 20 +++++++++++++++++--- 2 files changed, 17 insertions(+), 7 deletions(-)
ACK Jan
This looks okay to me. Reviewed-by: Farhan Ali <alifm@linux.vnet.ibm.com> On 12/06/2017 08:13 PM, 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 | 20 +++++++++++++++++--- 2 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ffa89aa4a..894c3f323 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 877a0a322..8d966156d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3893,8 +3893,14 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk)
static int -qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller) +qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) { + 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: @@ -3915,9 +3921,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: @@ -3957,7 +3969,8 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, break;
case VIR_DOMAIN_DEVICE_CONTROLLER: - ret = qemuDomainDeviceDefValidateController(dev->data.controller); + ret = qemuDomainDeviceDefValidateController(dev->data.controller, def, + qemuCaps); break;
case VIR_DOMAIN_DEVICE_LEASE: @@ -3978,6 +3991,7 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, break; }
+ virObjectUnref(qemuCaps); return ret; }
From: Lin Ma <lma@suse.com> Move the IDE controller check from command line building to controller def validation. Also explicitly include the avoidance check for the implicit IDE controller from qemuBuildSkipController. 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 | 33 ++++++++++++++++++++++++++++++++- 2 files changed, 32 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 894c3f323..2dd50a214 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 8d966156d..076c68c9f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3893,16 +3893,47 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk) static int +qemuDomainDeviceDefValidateControllerIDE(const virDomainControllerDef *controller, + const virDomainDef *def) +{ + /* first IDE controller is implicit on various machines */ + if (controller->idx == 0 && qemuDomainHasBuiltinIDE(def)) + return 0; + + /* 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; + if (!qemuDomainCheckCCWS390AddressSupport(def, controller->info, qemuCaps, "controller")) return -1; switch ((virDomainControllerType) controller->type) { case VIR_DOMAIN_CONTROLLER_TYPE_IDE: + ret = qemuDomainDeviceDefValidateControllerIDE(controller, def); + break; + case VIR_DOMAIN_CONTROLLER_TYPE_FDC: case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: case VIR_DOMAIN_CONTROLLER_TYPE_SATA: @@ -3914,7 +3945,7 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, break; } - return 0; + return ret; } -- 2.13.6
On Wed, Dec 06, 2017 at 08:14:00PM -0500, John Ferlan wrote:
From: Lin Ma <lma@suse.com>
Move the IDE controller check from command line building to controller def validation. Also explicitly include the avoidance check for the implicit IDE controller from qemuBuildSkipController.
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 | 33 ++++++++++++++++++++++++++++++++- 2 files changed, 32 insertions(+), 17 deletions(-)
ACK Jan
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 | 97 ++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 105 insertions(+), 86 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2dd50a214..cdd267675 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 076c68c9f..428db1193 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3918,6 +3918,97 @@ qemuDomainDeviceDefValidateControllerIDE(const virDomainControllerDef *controlle } +/* 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, @@ -3934,8 +4025,12 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, ret = qemuDomainDeviceDefValidateControllerIDE(controller, 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: -- 2.13.6
On Wed, Dec 06, 2017 at 08:14:01PM -0500, John Ferlan wrote:
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.
'While at it' in the commit message is usually an indicator of a change that should have been in a separate commit.
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 | 97 ++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 105 insertions(+), 86 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2dd50a214..cdd267675 100644
[...]
@@ -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)) {
These errors are reported for non-SCSI controllers too.
- 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;
After this patch, this function is called twice. Also it's called SetModel, while it's behaving as GetModel. Can we start assigning the default model in post-parse without breaking backwards compatibility? Jan
+ 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");
On 12/08/2017 10:37 AM, Ján Tomko wrote:
On Wed, Dec 06, 2017 at 08:14:01PM -0500, John Ferlan wrote:
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.
'While at it' in the commit message is usually an indicator of a change that should have been in a separate commit.
Sure... That's fine. Couple more patches added...
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 | 97 ++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 105 insertions(+), 86 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2dd50a214..cdd267675 100644
[...]
@@ -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; - }
Oh and of course I cannot remove this since this is called from hotplug code we need to perhaps alter the model here...
- - if (!(def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI && - model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI)) {
These errors are reported for non-SCSI controllers too.
Hmm.. true, dang compound conditionals...
- 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;
After this patch, this function is called twice. Also it's called SetModel, while it's behaving as GetModel. Can we start assigning the default model in post-parse without breaking backwards compatibility?
Jan
Are you asking that during PostParse callback for us to set def->model so that we can just use it later on? That'd make things a lot easier later on, but that seems a bit outside of what I was trying to do though. Anyway based on this series and another I have posted... That means for someone that didn't set the model, that a model would then be set and written out... Meaning adjustment of a number of tests because now there would be a model defined. It also could mean we cannot adjust the default model - one could say for example that using lsi as the default model for scsi_host passthrough is perhaps less optimal than using virtio-scsi... In fact, based on a different series and some testing done - it may not work either. As for GetModel vs. SetModel - I think that's a different problem and can be put on a list of things to do after this is done. Tks - John
+ 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");
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 pcie-root and pci-root avoidance from qemuBuildSkipController and the first switch found in qemuBuildControllerDevStr. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 20 -------------------- src/qemu/qemu_domain.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 48 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index cdd267675..e844dfe7c 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 428db1193..ceb03a0cd 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4010,6 +4010,50 @@ qemuDomainDeviceDefValidateControllerSCSI(const virDomainControllerDef *controll static int +qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controller, + const virDomainDef *def) +{ + virDomainControllerModelPCI model = controller->model; + + /* skip pcie-root */ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) + return 0; + + /* 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 0; + + 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) @@ -4030,12 +4074,15 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, qemuCaps); break; + case VIR_DOMAIN_CONTROLLER_TYPE_PCI: + ret = qemuDomainDeviceDefValidateControllerPCI(controller, def); + 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
On Wed, Dec 06, 2017 at 08:14:02PM -0500, John Ferlan wrote:
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 pcie-root and pci-root avoidance from qemuBuildSkipController and the first switch found in qemuBuildControllerDevStr.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 20 -------------------- src/qemu/qemu_domain.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 48 insertions(+), 21 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 428db1193..ceb03a0cd 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4010,6 +4010,50 @@ qemuDomainDeviceDefValidateControllerSCSI(const virDomainControllerDef *controll
static int +qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controller, + const virDomainDef *def) +{ + virDomainControllerModelPCI model = controller->model; + + /* skip pcie-root */ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
We already checked that the controller type is PCI in the caller.
+ controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) + return 0; + + /* 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 &&
Same here.
+ controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) + return 0; +
Jan
On 12/08/2017 10:38 AM, Ján Tomko wrote:
On Wed, Dec 06, 2017 at 08:14:02PM -0500, John Ferlan wrote:
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 pcie-root and pci-root avoidance from qemuBuildSkipController and the first switch found in qemuBuildControllerDevStr.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 20 -------------------- src/qemu/qemu_domain.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 48 insertions(+), 21 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 428db1193..ceb03a0cd 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4010,6 +4010,50 @@ qemuDomainDeviceDefValidateControllerSCSI(const virDomainControllerDef *controll
static int +qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controller, + const virDomainDef *def) +{ + virDomainControllerModelPCI model = controller->model; + + /* skip pcie-root */ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
We already checked that the controller type is PCI in the caller.
Oh right, hence my distaste for cut-n-paste code ;-) John
+ controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) + return 0; + + /* 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 &&
Same here.
+ controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) + return 0; +
Jan
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 e844dfe7c..4e292e446 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
On Wed, Dec 06, 2017 at 08:14:03PM -0500, John Ferlan wrote:
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(-)
ACK Jan
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 4e292e446..45cbf5381 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 ceb03a0cd..ecfff4209 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4014,6 +4014,7 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle const virDomainDef *def) { virDomainControllerModelPCI model = controller->model; + const virDomainPCIControllerOpts *pciopts; /* skip pcie-root */ if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && @@ -4049,6 +4050,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
On Wed, Dec 06, 2017 at 08:14:04PM -0500, John Ferlan wrote:
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.
The errors should all be unreachable. I don't see a need to make them more specific and bother the translators with changes in them.
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 4e292e446..45cbf5381 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 ceb03a0cd..ecfff4209 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4014,6 +4014,7 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle const virDomainDef *def) { virDomainControllerModelPCI model = controller->model; + const virDomainPCIControllerOpts *pciopts;
/* skip pcie-root */ if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && @@ -4049,6 +4050,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"),
%s not surrounded by any quotes. IMO 'autogenerated pci controller %option not set' should be enough for all these "errors" Jan
+ virDomainControllerModelPCITypeToString(controller->model)); + return -1; + } + } + return 0; }
-- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 12/08/2017 10:48 AM, Ján Tomko wrote:
On Wed, Dec 06, 2017 at 08:14:04PM -0500, John Ferlan wrote:
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.
The errors should all be unreachable. I don't see a need to make them more specific and bother the translators with changes in them.
Hmmm... I see... Although virDomainControllerDefParseXML does only parse them if they exist, the generation of <model name='%s'/> is "hidden", so splatting the error for what should never happen doesn't seem like a good idea. Similarly for the other options as well. So I'll make them all the same generic error message (even though it does seem a bit strange personally).
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 4e292e446..45cbf5381 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 ceb03a0cd..ecfff4209 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4014,6 +4014,7 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle const virDomainDef *def) { virDomainControllerModelPCI model = controller->model; + const virDomainPCIControllerOpts *pciopts;
/* skip pcie-root */ if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && @@ -4049,6 +4050,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"),
%s not surrounded by any quotes. IMO 'autogenerated pci controller %option not set' should be enough for all these "errors"
In this case the "autogenerated %s" wasn't single quoted before. I could single quote it, but that makes it "different" than before. I did check that all the %s names matched the virDomainControllerModelPCI list of names. I think it should stay without the single quote. John
Jan
+ virDomainControllerModelPCITypeToString(controller->model)); + return -1; + } + } + return 0; }
-- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 45cbf5381..e038abc9d 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 ecfff4209..050355fe3 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4015,6 +4015,7 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle { virDomainControllerModelPCI model = controller->model; const virDomainPCIControllerOpts *pciopts; + const char *modelName = NULL; /* skip pcie-root */ if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && @@ -4059,6 +4060,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 e038abc9d..92a7f8641 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 050355fe3..5007f0029 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4029,6 +4029,8 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) return 0; + /* 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: @@ -4071,6 +4073,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 | 80 +++++++++++++++++++++++++++++++++++++++++++++++-- tests/qemumemlocktest.c | 14 +++++++++ tests/qemuxml2xmltest.c | 5 +++- 4 files changed, 105 insertions(+), 72 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 92a7f8641..83431f458 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 5007f0029..0b2295129 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4011,7 +4011,8 @@ qemuDomainDeviceDefValidateControllerSCSI(const virDomainControllerDef *controll static int qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controller, - const virDomainDef *def) + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) { virDomainControllerModelPCI model = controller->model; const virDomainPCIControllerOpts *pciopts; @@ -4090,6 +4091,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: @@ -4107,6 +4115,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: @@ -4118,6 +4133,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: @@ -4130,6 +4152,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: @@ -4141,6 +4179,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: @@ -4160,6 +4205,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: @@ -4177,6 +4230,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: @@ -4198,6 +4258,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: @@ -4231,7 +4306,8 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, break; case VIR_DOMAIN_CONTROLLER_TYPE_PCI: - ret = qemuDomainDeviceDefValidateControllerPCI(controller, def); + ret = qemuDomainDeviceDefValidateControllerPCI(controller, def, + 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 2be8eb2c1..e866fb724 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
On Wed, Dec 06, 2017 at 08:14:07PM -0500, John Ferlan wrote:
Move the qemuCaps checks over to qemuDomainControllerDefValidatePCI and add empty lines between cases so it's a bit easier on the eyes to read.
Same comment as for 5/12. Jan
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 | 80 +++++++++++++++++++++++++++++++++++++++++++++++-- tests/qemumemlocktest.c | 14 +++++++++ tests/qemuxml2xmltest.c | 5 +++- 4 files changed, 105 insertions(+), 72 deletions(-)
On 12/08/2017 10:49 AM, Ján Tomko wrote:
On Wed, Dec 06, 2017 at 08:14:07PM -0500, John Ferlan wrote:
Move the qemuCaps checks over to qemuDomainControllerDefValidatePCI and add empty lines between cases so it's a bit easier on the eyes to read.
Same comment as for 5/12.
Jan
For this one it's overkill IMO to generate another patch to add the spaces, so I'll just drop them as they weren't important other than aesthetics. Tks - John
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 | 80 +++++++++++++++++++++++++++++++++++++++++++++++-- tests/qemumemlocktest.c | 14 +++++++++ tests/qemuxml2xmltest.c | 5 +++- 4 files changed, 105 insertions(+), 72 deletions(-)
Move the SATA controller check from command line building to controller def validation. This includes copying the SATA skip check found in qemuBuildSkipController. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 6 ------ src/qemu/qemu_domain.c | 25 ++++++++++++++++++++++++- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 83431f458..f41b42aac 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 0b2295129..159876279 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4285,6 +4285,25 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle static int +qemuDomainDeviceDefValidateControllerSATA(const virDomainControllerDef *controller, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + /* first SATA controller on Q35 machines is implicit */ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA && + controller->idx == 0 && qemuDomainIsQ35(def)) + return 0; + + 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) @@ -4310,8 +4329,12 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, qemuCaps); break; - case VIR_DOMAIN_CONTROLLER_TYPE_FDC: case VIR_DOMAIN_CONTROLLER_TYPE_SATA: + ret = qemuDomainDeviceDefValidateControllerSATA(controller, def, + 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
On Wed, Dec 06, 2017 at 08:14:08PM -0500, John Ferlan wrote:
Move the SATA controller check from command line building to controller def validation. This includes copying the SATA skip check found in qemuBuildSkipController.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 6 ------ src/qemu/qemu_domain.c | 25 ++++++++++++++++++++++++- 2 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 83431f458..f41b42aac 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 0b2295129..159876279 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4285,6 +4285,25 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle
static int +qemuDomainDeviceDefValidateControllerSATA(const virDomainControllerDef *controller, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + /* first SATA controller on Q35 machines is implicit */ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA &&
The type is _SATA, it's in the name fo this function Jan
+ controller->idx == 0 && qemuDomainIsQ35(def)) + return 0; + + 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)
participants (3)
-
Farhan Ali -
John Ferlan -
Ján Tomko