[libvirt] [PATCH v5 00/16] Move qemu command line controller checks to qemuDomainDeviceDefValidateController* checks
v4: https://www.redhat.com/archives/libvir-list/2017-December/msg00401.html Changes since v4: Insert patch 1 to split the qemuDomainSetSCSIControllerModel as has been discussed during review. Adjustments to the SCSI patches as a result. Added patch 16 from Mark Hartmayer Andrea Bolognani (1): qemu: Add missing checks for pcie-root-port options John Ferlan (14): qemu: Split qemuDomainSetSCSIControllerModel qemu: Introduce qemuDomainDeviceDefValidateControllerAttributes qemu: Move and rename qemuBuildCheckSCSIControllerModel qemu: Move model set for qemuBuildControllerDevStr qemu: Adjust SCSI controller switch in qemuBuildControllerDevStr qemu: Add check for iothread attribute in validate controller 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 Marc Hartmayer (1): qemu: Use switch statement for address types in qemuBuildControllerDevStr src/qemu/qemu_alias.c | 4 +- src/qemu/qemu_command.c | 452 +++++---------------------------------- src/qemu/qemu_domain.c | 474 ++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_domain_address.c | 74 ++----- src/qemu/qemu_domain_address.h | 6 +- tests/qemumemlocktest.c | 14 ++ tests/qemuxml2xmltest.c | 5 +- 7 files changed, 568 insertions(+), 461 deletions(-) -- 2.13.6
Rather than one function serving two purposes, let's split things up into qemuDomainResetSCSIControllerModel for all current callers and then add qemuDomainCheckSCSIControllerModel when building the controller command line to check the capabilities. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_alias.c | 4 +-- src/qemu/qemu_command.c | 62 ++++++++++++++++++++++++++++++++--- src/qemu/qemu_domain_address.c | 74 +++++++++--------------------------------- src/qemu/qemu_domain_address.h | 6 ++-- 4 files changed, 79 insertions(+), 67 deletions(-) diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 37fe2aa80..b65276dd9 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -194,8 +194,8 @@ qemuAssignDeviceDiskAlias(virDomainDefPtr def, virDomainDeviceFindControllerModel(def, &disk->info, VIR_DOMAIN_CONTROLLER_TYPE_SCSI); - if ((qemuDomainSetSCSIControllerModel(def, qemuCaps, - &controllerModel)) < 0) + if ((qemuDomainResetSCSIControllerModel(def, qemuCaps, + &controllerModel)) < 0) return -1; } diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b8aede32d..5c084ae8c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1888,6 +1888,57 @@ qemuCheckIOThreads(const virDomainDef *def, } +static bool +qemuBuildCheckSCSIControllerModel(virQEMUCapsPtr qemuCaps, + int model) +{ + switch (model) { + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_LSI)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support " + "the LSI 53C895A SCSI controller")); + return false; + } + break; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support " + "virtio scsi controller")); + return false; + } + break; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI: + /*TODO: need checking work here if necessary */ + break; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_MPTSAS1068)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support " + "the LSI SAS1068 (MPT Fusion) controller")); + return false; + } + break; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_MEGASAS)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support " + "the LSI SAS1078 (MegaRAID) controller")); + return false; + } + break; + default: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported controller model: %s"), + virDomainControllerModelSCSITypeToString(model)); + return false; + } + + return true; +} + + char * qemuBuildDriveDevStr(const virDomainDef *def, virDomainDiskDefPtr disk, @@ -1983,8 +2034,8 @@ qemuBuildDriveDevStr(const virDomainDef *def, controllerModel = virDomainDeviceFindControllerModel(def, &disk->info, VIR_DOMAIN_CONTROLLER_TYPE_SCSI); - if ((qemuDomainSetSCSIControllerModel(def, qemuCaps, - &controllerModel)) < 0) + if ((qemuDomainResetSCSIControllerModel(def, qemuCaps, + &controllerModel)) < 0) goto error; if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { @@ -2663,7 +2714,10 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, *devstr = NULL; if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { - if ((qemuDomainSetSCSIControllerModel(domainDef, qemuCaps, &model)) < 0) + if ((qemuDomainResetSCSIControllerModel(domainDef, qemuCaps, &model)) < 0) + return -1; + + if (!qemuBuildCheckSCSIControllerModel(qemuCaps, model)) return -1; } @@ -5084,7 +5138,7 @@ qemuBuildSCSIHostdevDevStr(const virDomainDef *def, model = virDomainDeviceFindControllerModel(def, dev->info, VIR_DOMAIN_CONTROLLER_TYPE_SCSI); - if (qemuDomainSetSCSIControllerModel(def, qemuCaps, &model) < 0) + if (qemuDomainResetSCSIControllerModel(def, qemuCaps, &model) < 0) goto error; if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC) { diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 6e7561d39..e3c1760d8 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -41,65 +41,23 @@ VIR_LOG_INIT("qemu.qemu_domain_address"); int -qemuDomainSetSCSIControllerModel(const virDomainDef *def, - virQEMUCapsPtr qemuCaps, - int *model) +qemuDomainResetSCSIControllerModel(const virDomainDef *def, + virQEMUCapsPtr qemuCaps, + int *model) { - if (*model > 0) { - switch (*model) { - case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_LSI)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("This QEMU doesn't support " - "the LSI 53C895A SCSI controller")); - return -1; - } - break; - case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("This QEMU doesn't support " - "virtio scsi controller")); - return -1; - } - break; - case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI: - /*TODO: need checking work here if necessary */ - break; - case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_MPTSAS1068)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("This QEMU doesn't support " - "the LSI SAS1068 (MPT Fusion) controller")); - return -1; - } - break; - case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_MEGASAS)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("This QEMU doesn't support " - "the LSI SAS1078 (MegaRAID) controller")); - return -1; - } - break; - default: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unsupported controller model: %s"), - virDomainControllerModelSCSITypeToString(*model)); - return -1; - } + if (*model > 0) + return 0; + + if (qemuDomainIsPSeries(def)) { + *model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI; + } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_LSI)) { + *model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC; + } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI)) { + *model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI; } else { - if (qemuDomainIsPSeries(def)) { - *model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI; - } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_LSI)) { - *model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC; - } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI)) { - *model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to determine model for scsi controller")); - return -1; - } + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to determine model for scsi controller")); + return -1; } return 0; @@ -230,7 +188,7 @@ qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def, model = cont->model; if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { - if (qemuDomainSetSCSIControllerModel(def, qemuCaps, &model) < 0) + if (qemuDomainResetSCSIControllerModel(def, qemuCaps, &model) < 0) goto cleanup; } diff --git a/src/qemu/qemu_domain_address.h b/src/qemu/qemu_domain_address.h index e951a4c88..5a2cbe39d 100644 --- a/src/qemu/qemu_domain_address.h +++ b/src/qemu/qemu_domain_address.h @@ -28,9 +28,9 @@ # include "qemu_conf.h" # include "qemu_capabilities.h" -int qemuDomainSetSCSIControllerModel(const virDomainDef *def, - virQEMUCapsPtr qemuCaps, - int *model); +int qemuDomainResetSCSIControllerModel(const virDomainDef *def, + virQEMUCapsPtr qemuCaps, + int *model); int qemuDomainAssignAddresses(virDomainDefPtr def, virQEMUCapsPtr qemuCaps, -- 2.13.6
On 01/06/2018 12:47 AM, John Ferlan wrote:
Rather than one function serving two purposes, let's split things up into qemuDomainResetSCSIControllerModel for all current callers and then add qemuDomainCheckSCSIControllerModel when building the controller command line to check the capabilities.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_alias.c | 4 +-- src/qemu/qemu_command.c | 62 ++++++++++++++++++++++++++++++++--- src/qemu/qemu_domain_address.c | 74 +++++++++--------------------------------- src/qemu/qemu_domain_address.h | 6 ++-- 4 files changed, 79 insertions(+), 67 deletions(-)
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 37fe2aa80..b65276dd9 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -194,8 +194,8 @@ qemuAssignDeviceDiskAlias(virDomainDefPtr def, virDomainDeviceFindControllerModel(def, &disk->info, VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
- if ((qemuDomainSetSCSIControllerModel(def, qemuCaps, - &controllerModel)) < 0) + if ((qemuDomainResetSCSIControllerModel(def, qemuCaps, + &controllerModel)) < 0) return -1; }
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b8aede32d..5c084ae8c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1888,6 +1888,57 @@ qemuCheckIOThreads(const virDomainDef *def, }
+static bool +qemuBuildCheckSCSIControllerModel(virQEMUCapsPtr qemuCaps, + int model) +{ + switch (model) { + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_LSI)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support " + "the LSI 53C895A SCSI controller")); + return false; + } + break; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support " + "virtio scsi controller")); + return false; + } + break; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI: + /*TODO: need checking work here if necessary */ + break; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_MPTSAS1068)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support " + "the LSI SAS1068 (MPT Fusion) controller")); + return false; + } + break; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_MEGASAS)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support " + "the LSI SAS1078 (MegaRAID) controller")); + return false; + } + break; + default: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported controller model: %s"), + virDomainControllerModelSCSITypeToString(model)); + return false;
Or, just have this function take virDomainControllerModelSCSI enum and instead of default have VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST and probably VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO; That way, when new model is added compiler identifies this place for adjustment automatically.
+ } + + return true; +}
ACK Michal
On 01/28/2018 03:48 AM, Michal Privoznik wrote:
On 01/06/2018 12:47 AM, John Ferlan wrote:
Rather than one function serving two purposes, let's split things up into qemuDomainResetSCSIControllerModel for all current callers and then add qemuDomainCheckSCSIControllerModel when building the controller command line to check the capabilities.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_alias.c | 4 +-- src/qemu/qemu_command.c | 62 ++++++++++++++++++++++++++++++++--- src/qemu/qemu_domain_address.c | 74 +++++++++--------------------------------- src/qemu/qemu_domain_address.h | 6 ++-- 4 files changed, 79 insertions(+), 67 deletions(-)
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 37fe2aa80..b65276dd9 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -194,8 +194,8 @@ qemuAssignDeviceDiskAlias(virDomainDefPtr def, virDomainDeviceFindControllerModel(def, &disk->info, VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
- if ((qemuDomainSetSCSIControllerModel(def, qemuCaps, - &controllerModel)) < 0) + if ((qemuDomainResetSCSIControllerModel(def, qemuCaps, + &controllerModel)) < 0) return -1; }
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b8aede32d..5c084ae8c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1888,6 +1888,57 @@ qemuCheckIOThreads(const virDomainDef *def, }
+static bool +qemuBuildCheckSCSIControllerModel(virQEMUCapsPtr qemuCaps, + int model) +{ + switch (model) {
Change to: switch ((virDomainControllerModelSCSI) model) {
+ case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_LSI)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support " + "the LSI 53C895A SCSI controller")); + return false; + } + break; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support " + "virtio scsi controller")); + return false; + } + break; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI: + /*TODO: need checking work here if necessary */ + break; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_MPTSAS1068)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support " + "the LSI SAS1068 (MPT Fusion) controller")); + return false; + } + break; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_MEGASAS)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support " + "the LSI SAS1078 (MegaRAID) controller")); + return false; + } + break; + default:
Change to: 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(model)); + return false;
Or, just have this function take virDomainControllerModelSCSI enum and instead of default have VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST and probably VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO; That way, when new model is added compiler identifies this place for adjustment automatically.
NB: Went with cast-ing the switch rather than changing from int model to virDomainControllerModelSCSI model - mainly because the callers define parameter as int...
+ } + + return true; +}
ACK
Michal
Tks - John
Move the checks that various attributes are not set on any controller other than SCSI controller using virtio-scsi model into the common controller validate checks. Need to also add a qemuDomainResetSCSIControllerModel call in order to ensure we get the "right" SCSI model if it's not set by default since it wouldn't be set during post parse processing. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 24 ------------------------ src/qemu/qemu_domain.c | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5c084ae8c..1a9249e1b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2721,30 +2721,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, 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) { diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index de46ab996..1a890c01a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3941,6 +3941,38 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk) static int +qemuDomainDeviceDefValidateControllerAttributes(const virDomainControllerDef *controller, + int model) +{ + if (!(controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI && + 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; + } + } + + return 0; +} + + +static int qemuDomainDeviceDefValidateControllerIDE(const virDomainControllerDef *controller, const virDomainDef *def) { @@ -3972,11 +4004,20 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, virQEMUCapsPtr qemuCaps) { int ret = 0; + int model = controller->model; if (!qemuDomainCheckCCWS390AddressSupport(def, controller->info, qemuCaps, "controller")) return -1; + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { + if ((qemuDomainResetSCSIControllerModel(def, qemuCaps, &model)) < 0) + return -1; + } + + if (qemuDomainDeviceDefValidateControllerAttributes(controller, model) < 0) + return -1; + switch ((virDomainControllerType) controller->type) { case VIR_DOMAIN_CONTROLLER_TYPE_IDE: ret = qemuDomainDeviceDefValidateControllerIDE(controller, def); -- 2.13.6
On 01/06/2018 12:47 AM, John Ferlan wrote:
Move the checks that various attributes are not set on any controller other than SCSI controller using virtio-scsi model into the common controller validate checks.
Need to also add a qemuDomainResetSCSIControllerModel call in order to ensure we get the "right" SCSI model if it's not set by default since it wouldn't be set during post parse processing.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 24 ------------------------ src/qemu/qemu_domain.c | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 24 deletions(-)
The only problem I have with this approach is that while previously we've checked for QEMU caps at domain start time, now we check for them at define time. So I guess in general it's not a safe thing to do. For instance, I'd be against moving all checks done at cmd line time to DefPostParse as they introduce TOCTOU problem. However, some checks (mostly semantic ones) can be done in post parse callbacks. For example, trying to plug a disk onto ISA bus will fail regardless of qemu caps. However, whether qemu supports VIRTIO_SCSI or not should not matter at define time as this might change after domain is defined. However, SCSI controllers have been around for quite some time, so unless somebody is upgrading from ancient qemu, we are safe. ACK Michal
On Sun, Jan 28, 2018 at 09:48:19AM +0100, Michal Privoznik wrote:
On 01/06/2018 12:47 AM, John Ferlan wrote:
Move the checks that various attributes are not set on any controller other than SCSI controller using virtio-scsi model into the common controller validate checks.
Need to also add a qemuDomainResetSCSIControllerModel call in order to ensure we get the "right" SCSI model if it's not set by default since it wouldn't be set during post parse processing.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 24 ------------------------ src/qemu/qemu_domain.c | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 24 deletions(-)
The only problem I have with this approach is that while previously we've checked for QEMU caps at domain start time, now we check for them at define time. So I guess in general it's not a safe thing to do.
For instance, I'd be against moving all checks done at cmd line time to DefPostParse as they introduce TOCTOU problem.
This is not DefPostParse, this is DefValidate. PostParse is called on all XML parsing (and as of <7c5cf4983> allowed to fail and then re-run on domain startup). Validate is run when defining some new domains (the _VALIDATE flag has to be added to the APIs) and then unconditionally on domain startup. So the only problem here would be that we might not allow to define a domain until you install QEMU with the requested features.
However, some checks (mostly semantic ones) can be done in post parse callbacks. For example, trying to plug a disk onto ISA bus will fail regardless of qemu caps. However, whether qemu supports VIRTIO_SCSI or not should not matter at define time as this might change after domain is defined.
However, SCSI controllers have been around for quite some time, so unless somebody is upgrading from ancient qemu, we are safe.
Generally we try not to break parsing existing configs (even with unusable domains), which is why Validate functions are separate from PostParse. Jan
ACK
Michal
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/29/2018 04:28 AM, Ján Tomko wrote:
On Sun, Jan 28, 2018 at 09:48:19AM +0100, Michal Privoznik wrote:
On 01/06/2018 12:47 AM, John Ferlan wrote:
Move the checks that various attributes are not set on any controller other than SCSI controller using virtio-scsi model into the common controller validate checks.
Need to also add a qemuDomainResetSCSIControllerModel call in order to ensure we get the "right" SCSI model if it's not set by default since it wouldn't be set during post parse processing.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 24 ------------------------ src/qemu/qemu_domain.c | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 24 deletions(-)
The only problem I have with this approach is that while previously we've checked for QEMU caps at domain start time, now we check for them at define time. So I guess in general it's not a safe thing to do.
For instance, I'd be against moving all checks done at cmd line time to DefPostParse as they introduce TOCTOU problem.
This is not DefPostParse, this is DefValidate.
PostParse is called on all XML parsing (and as of <7c5cf4983> allowed to fail and then re-run on domain startup).
Validate is run when defining some new domains (the _VALIDATE flag has to be added to the APIs) and then unconditionally on domain startup.
While I didn't go chase the commit for validate to be enabled, this is essentially my understanding of validate too. Callbacks that while could be in post parse callbacks, we don't put there because we don't want running domains to disappear. The whole post parse and validate and flags can be really confusing....
So the only problem here would be that we might not allow to define a domain until you install QEMU with the requested features.
True, but I don't believe it wouldn't be a running domain that would disappear as opposed to if these were post parse callback checks. Conversely this may "help" in the detection of some invalid configuration on a future change and test at XML2XML processing time rather than XML2ARGV (and don't really run the domain) time.
However, some checks (mostly semantic ones) can be done in post parse callbacks. For example, trying to plug a disk onto ISA bus will fail regardless of qemu caps. However, whether qemu supports VIRTIO_SCSI or not should not matter at define time as this might change after domain is defined.
However, SCSI controllers have been around for quite some time, so unless somebody is upgrading from ancient qemu, we are safe.
Generally we try not to break parsing existing configs (even with unusable domains), which is why Validate functions are separate from PostParse.
Jan
ACK
Michal
Having the ACK and an open question/issue is making me "pause"... I'd like to resolve Jan's other point as well, but I'll respond to it separately. John
On Fri, Jan 05, 2018 at 06:47:25PM -0500, John Ferlan wrote:
Move the checks that various attributes are not set on any controller other than SCSI controller using virtio-scsi model into the common controller validate checks.
Need to also add a qemuDomainResetSCSIControllerModel call in order to ensure we get the "right" SCSI model if it's not set by default since it wouldn't be set during post parse processing.
So we should set it in post-parse processing once instead of adding this second call. Jan
On 01/29/2018 04:18 AM, Ján Tomko wrote:
On Fri, Jan 05, 2018 at 06:47:25PM -0500, John Ferlan wrote:
Move the checks that various attributes are not set on any controller other than SCSI controller using virtio-scsi model into the common controller validate checks.
Need to also add a qemuDomainResetSCSIControllerModel call in order to ensure we get the "right" SCSI model if it's not set by default since it wouldn't be set during post parse processing.
So we should set it in post-parse processing once instead of adding this second call.
Jan
This is a point you raised in v3: https://www.redhat.com/archives/libvir-list/2017-December/msg00293.html to which I responded: https://www.redhat.com/archives/libvir-list/2017-December/msg00304.html and even carried into my v4: https://www.redhat.com/archives/libvir-list/2018-January/msg00103.html So, now that we're here again - the question becomes, do we really want post parse processing to set a default model if it's not set? Should that be part of this series? Should it be a followup? Should be something done before this one and then rework this one? Are there drawbacks to doing so? One drawback that I kept coming back to is that LSI becomes the default qemu model instead of VIRTIO-SCSI (it's the order of setting *model in qemuDomainSetSCSIControllerModel). That drawback caused problems when setting the model if we create a new SCSI controller during something like hotplug because we had already filled the current controller or when adding one VIRTIO-SCSI controller but more than 7 disks (or hostdevs) that end up using that controller. Although I think that's at least partially resolved by other changes I've already made. Tks - John
On Mon, Jan 29, 2018 at 12:43:14PM -0500, John Ferlan wrote:
On 01/29/2018 04:18 AM, Ján Tomko wrote:
On Fri, Jan 05, 2018 at 06:47:25PM -0500, John Ferlan wrote:
Move the checks that various attributes are not set on any controller other than SCSI controller using virtio-scsi model into the common controller validate checks.
Need to also add a qemuDomainResetSCSIControllerModel call in order to ensure we get the "right" SCSI model if it's not set by default since it wouldn't be set during post parse processing.
So we should set it in post-parse processing once instead of adding this second call.
Jan
This is a point you raised in v3:
https://www.redhat.com/archives/libvir-list/2017-December/msg00293.html
to which I responded:
https://www.redhat.com/archives/libvir-list/2017-December/msg00304.html
and even carried into my v4:
https://www.redhat.com/archives/libvir-list/2018-January/msg00103.html
So, now that we're here again - the question becomes, do we really want post parse processing to set a default model if it's not set?
Yes, the used model should be recorded in the XML.
Should that be part of this series? Should it be a followup? Should be something done before this one and then rework this one?
Ideally a prerequisite. If that's too much yak shaving for you, at least add a comment to the validate function saying picking a default model really does not belong in a validation function.
Are there drawbacks to doing so? One drawback that I kept coming back to is that LSI becomes the default qemu model instead of VIRTIO-SCSI (it's the order of setting *model in qemuDomainSetSCSIControllerModel).
When was virtio-scsi the default model? Now we'd just record the picked default in XML. Jan
That drawback caused problems when setting the model if we create a new SCSI controller during something like hotplug because we had already filled the current controller or when adding one VIRTIO-SCSI controller but more than 7 disks (or hostdevs) that end up using that controller. Although I think that's at least partially resolved by other changes I've already made.
Tks -
John
On 01/29/2018 12:53 PM, Ján Tomko wrote:
On Mon, Jan 29, 2018 at 12:43:14PM -0500, John Ferlan wrote:
On 01/29/2018 04:18 AM, Ján Tomko wrote:
On Fri, Jan 05, 2018 at 06:47:25PM -0500, John Ferlan wrote:
Move the checks that various attributes are not set on any controller other than SCSI controller using virtio-scsi model into the common controller validate checks.
Need to also add a qemuDomainResetSCSIControllerModel call in order to ensure we get the "right" SCSI model if it's not set by default since it wouldn't be set during post parse processing.
So we should set it in post-parse processing once instead of adding this second call.
Jan
This is a point you raised in v3:
https://www.redhat.com/archives/libvir-list/2017-December/msg00293.html
to which I responded:
https://www.redhat.com/archives/libvir-list/2017-December/msg00304.html
and even carried into my v4:
https://www.redhat.com/archives/libvir-list/2018-January/msg00103.html
So, now that we're here again - the question becomes, do we really want post parse processing to set a default model if it's not set?
Yes, the used model should be recorded in the XML.
Should that be part of this series? Should it be a followup? Should be something done before this one and then rework this one?
Ideally a prerequisite. If that's too much yak shaving for you, at least add a comment to the validate function saying picking a default model really does not belong in a validation function.
As usual a seemingly simple request sends me down the rabbit hole. Setting a default controller model in qemuDomainControllerDefPostParse would require passing @qemuCaps - simple enough until one checks out the caller qemuDomainDeviceDefPostParse which notes: " /* Note that qemuCaps may be NULL when this function is called. This * function shall not fail in that case. It will be re-run on VM startup * with the capabilities populated. */ " <sigh> So, would adding the following to patch 1 for the Reset function suffice? /** qemuDomainResetSCSIControllerModel * @def: Domain def * @qemuCaps: QEMU capabilities * @model: model to reset "default" to - either by @def or @qemuCaps * * Typically qemuDomainControllerDefPostParse would be used to set * a default controller model; however, qemuDomainDeviceDefPostParse * can be passed a NULL @qemuCaps, so setting the default model may * not be possible. Thus we're left to calling this Reset function * from numerous locations that need to get the default model for * the controller when one is not defined. * * Returns 0 on success, -1 on failure */ I can also add a note in qemuDomainDeviceDefValidateController that restates this for this patch, e.g.: if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { /* Resetting the default should have been handled during post parse * parse callback; however, at that time we could not guarantee that * qemuCaps was valid, so we're left doing this now */ if ((qemuDomainResetSCSIControllerModel(def, qemuCaps, &model)) < 0) return -1; }
Are there drawbacks to doing so? One drawback that I kept coming back to is that LSI becomes the default qemu model instead of VIRTIO-SCSI (it's the order of setting *model in qemuDomainSetSCSIControllerModel).
When was virtio-scsi the default model? Now we'd just record the picked default in XML.
virtio-scsi was never the default AFAIK... That wasn't the point/problem. The problem is described in the next paragraph.
Jan
That drawback caused problems when setting the model if we create a new SCSI controller during something like hotplug because we had already filled the current controller or when adding one VIRTIO-SCSI controller but more than 7 disks (or hostdevs) that end up using that controller. Although I think that's at least partially resolved by other changes I've already made.
I looked it up, this is handled by c52dbafe and 07beea6c John
On Mon, Jan 29, 2018 at 16:18:46 -0500, John Ferlan wrote:
On 01/29/2018 12:53 PM, Ján Tomko wrote:
On Mon, Jan 29, 2018 at 12:43:14PM -0500, John Ferlan wrote:
On 01/29/2018 04:18 AM, Ján Tomko wrote:
On Fri, Jan 05, 2018 at 06:47:25PM -0500, John Ferlan wrote:
Move the checks that various attributes are not set on any controller other than SCSI controller using virtio-scsi model into the common controller validate checks.
Need to also add a qemuDomainResetSCSIControllerModel call in order to ensure we get the "right" SCSI model if it's not set by default since it wouldn't be set during post parse processing.
So we should set it in post-parse processing once instead of adding this second call.
Jan
This is a point you raised in v3:
https://www.redhat.com/archives/libvir-list/2017-December/msg00293.html
to which I responded:
https://www.redhat.com/archives/libvir-list/2017-December/msg00304.html
and even carried into my v4:
https://www.redhat.com/archives/libvir-list/2018-January/msg00103.html
So, now that we're here again - the question becomes, do we really want post parse processing to set a default model if it's not set?
Yes, the used model should be recorded in the XML.
Should that be part of this series? Should it be a followup? Should be something done before this one and then rework this one?
Ideally a prerequisite. If that's too much yak shaving for you, at least add a comment to the validate function saying picking a default model really does not belong in a validation function.
Well it really should not be there. I've tried to make sure that at least the top level functions get a const domain definition, so that nobody even thinks about doing this. Obviously for members of the domain object this can't be enforced by the compiler. Only via code review. This is the prototype for the validator: static int qemuDomainDefValidate(const virDomainDef *def, virCapsPtr caps ATTRIBUTE_UNUSED, void *opaque) Do NOT try to set stuff there. The functions are meant only to check config.
As usual a seemingly simple request sends me down the rabbit hole. Setting a default controller model in qemuDomainControllerDefPostParse would require passing @qemuCaps - simple enough until one checks out the caller qemuDomainDeviceDefPostParse which notes:
" /* Note that qemuCaps may be NULL when this function is called. This * function shall not fail in that case. It will be re-run on VM startup * with the capabilities populated. */
I think I've seen this explained recently somewhere but I can reiterate. The above statement applies to cases when the postparse callback is called on code paths which should not fail. This includes reloading configs from disks and a few other things. qemuCaps will be NULL if the emulator binary is not present. We can't do much in that case, but we certainly should not drop domains. Defining new domains still requires qemuCaps to be populated so that defaults can be loaded according to the qemu capabilities. In cases when qemuCaps are NULL most of the defaults should be present. In cases when we would do this during daemon upgrade, post parse will be re-run during VM startup when qemuCaps are required to be present anyways.
[...]
Ideally a prerequisite. If that's too much yak shaving for you, at least add a comment to the validate function saying picking a default model really does not belong in a validation function.
Well it really should not be there. I've tried to make sure that at least the top level functions get a const domain definition, so that nobody even thinks about doing this.
Obviously for members of the domain object this can't be enforced by the compiler. Only via code review.
This is the prototype for the validator:
static int qemuDomainDefValidate(const virDomainDef *def, virCapsPtr caps ATTRIBUTE_UNUSED, void *opaque)
Do NOT try to set stuff there. The functions are meant only to check config.
Have no worries, the patches don't "set" the def->controller[n]->model value for SCSI controllers that don't have one set... If they did there would be a bunch of make check failures because a value would be set and thus alter output in some xml file the xml2xml testing. /me wondering if a make check rule should be made to ensure that const never changes...
As usual a seemingly simple request sends me down the rabbit hole. Setting a default controller model in qemuDomainControllerDefPostParse would require passing @qemuCaps - simple enough until one checks out the caller qemuDomainDeviceDefPostParse which notes:
" /* Note that qemuCaps may be NULL when this function is called. This * function shall not fail in that case. It will be re-run on VM startup * with the capabilities populated. */
I think I've seen this explained recently somewhere but I can reiterate.
The above statement applies to cases when the postparse callback is called on code paths which should not fail. This includes reloading configs from disks and a few other things.
qemuCaps will be NULL if the emulator binary is not present. We can't do much in that case, but we certainly should not drop domains.
Right, but if we cannot be sure that post parse would set the default, then I would think paths that possibly rely on getting a caps based model value cannot assume that the model was set during post parse. IOW: They'd have to adjust too - as seen with the USB and Net model code paths.
Defining new domains still requires qemuCaps to be populated so that defaults can be loaded according to the qemu capabilities. In cases when qemuCaps are NULL most of the defaults should be present. In cases when we would do this during daemon upgrade, post parse will be re-run during VM startup when qemuCaps are required to be present anyways.
Does today's truth hold true to some future? The API mentions that qemuCaps "may" be NULL, but provides no real guidance over what assumptions can be made by not setting something based on not having the capabilities existing. In any case, I'm working through some patches - let's see which rabbit hole I get stuck in. John
Move to qemu_domain during the validation of controller options and rename qemuDomainCheckSCSIControllerModel. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 54 ------------------------------------------------- src/qemu/qemu_domain.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 54 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1a9249e1b..0583f8fec 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1888,57 +1888,6 @@ qemuCheckIOThreads(const virDomainDef *def, } -static bool -qemuBuildCheckSCSIControllerModel(virQEMUCapsPtr qemuCaps, - int model) -{ - switch (model) { - case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_LSI)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("This QEMU doesn't support " - "the LSI 53C895A SCSI controller")); - return false; - } - break; - case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("This QEMU doesn't support " - "virtio scsi controller")); - return false; - } - break; - case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI: - /*TODO: need checking work here if necessary */ - break; - case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_MPTSAS1068)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("This QEMU doesn't support " - "the LSI SAS1068 (MPT Fusion) controller")); - return false; - } - break; - case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_MEGASAS)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("This QEMU doesn't support " - "the LSI SAS1078 (MegaRAID) controller")); - return false; - } - break; - default: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unsupported controller model: %s"), - virDomainControllerModelSCSITypeToString(model)); - return false; - } - - return true; -} - - char * qemuBuildDriveDevStr(const virDomainDef *def, virDomainDiskDefPtr disk, @@ -2716,9 +2665,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { if ((qemuDomainResetSCSIControllerModel(domainDef, qemuCaps, &model)) < 0) return -1; - - if (!qemuBuildCheckSCSIControllerModel(qemuCaps, model)) - return -1; } switch ((virDomainControllerType) def->type) { diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1a890c01a..a253d4708 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3972,6 +3972,57 @@ qemuDomainDeviceDefValidateControllerAttributes(const virDomainControllerDef *co } +static bool +qemuDomainCheckSCSIControllerModel(virQEMUCapsPtr qemuCaps, + int model) +{ + switch (model) { + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_LSI)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support " + "the LSI 53C895A SCSI controller")); + return false; + } + break; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support " + "virtio scsi controller")); + return false; + } + break; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI: + /*TODO: need checking work here if necessary */ + break; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_MPTSAS1068)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support " + "the LSI SAS1068 (MPT Fusion) controller")); + return false; + } + break; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_MEGASAS)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support " + "the LSI SAS1078 (MegaRAID) controller")); + return false; + } + break; + default: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported controller model: %s"), + virDomainControllerModelSCSITypeToString(model)); + return false; + } + + return true; +} + + static int qemuDomainDeviceDefValidateControllerIDE(const virDomainControllerDef *controller, const virDomainDef *def) @@ -4013,6 +4064,9 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { if ((qemuDomainResetSCSIControllerModel(def, qemuCaps, &model)) < 0) return -1; + + if (!qemuDomainCheckSCSIControllerModel(qemuCaps, model)) + return -1; } if (qemuDomainDeviceDefValidateControllerAttributes(controller, model) < 0) -- 2.13.6
On 01/06/2018 12:47 AM, John Ferlan wrote:
Move to qemu_domain during the validation of controller options and rename qemuDomainCheckSCSIControllerModel.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 54 ------------------------------------------------- src/qemu/qemu_domain.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 54 deletions(-)
ACK Michal
We can move the @model reset inside the switch case for VIR_DOMAIN_CONTROLLER_TYPE_SCSI since it is the only case that uses an altered model name. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0583f8fec..b955fb66e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2662,13 +2662,11 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, *devstr = NULL; - if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { + switch ((virDomainControllerType) def->type) { + case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: if ((qemuDomainResetSCSIControllerModel(domainDef, qemuCaps, &model)) < 0) return -1; - } - switch ((virDomainControllerType) def->type) { - case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: switch (model) { case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { -- 2.13.6
Modify the SCSI controller switch during command line building to account for all virDomainControllerModelSCSI types rather than using the default label. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b955fb66e..515206261 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2667,7 +2667,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, if ((qemuDomainResetSCSIControllerModel(domainDef, qemuCaps, &model)) < 0) return -1; - switch (model) { + 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"); @@ -2707,7 +2707,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)); -- 2.13.6
On 01/06/2018 12:47 AM, John Ferlan wrote:
Modify the SCSI controller switch during command line building to account for all virDomainControllerModelSCSI types rather than using the default label.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b955fb66e..515206261 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2667,7 +2667,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, if ((qemuDomainResetSCSIControllerModel(domainDef, qemuCaps, &model)) < 0) return -1;
- switch (model) { + 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"); @@ -2707,7 +2707,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));
ACK Michal
Let's make sure that non SCSI virtio-scsi isn't used for any type other than a virtio-scsi controller. This includes removing the check from qemuCheckSCSIControllerIOThreads which is a very suble difference because although def->model was used in the original comparison and just @model is used in the new comparison, the comparison is the same. This is because qemuDomainSetSCSIControllerModel doesn't change the def->model, thus we know that the resultant @model would result in either the same as input or if not set, whatever the default model is when def->model == -1. In this second case, virtio-scsi is the last default chosen. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 8 -------- src/qemu/qemu_domain.c | 5 +++++ 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 515206261..732c720b0 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2601,14 +2601,6 @@ qemuCheckSCSIControllerIOThreads(const virDomainDef *domainDef, 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", diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a253d4708..f60c6540e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3966,6 +3966,11 @@ qemuDomainDeviceDefValidateControllerAttributes(const virDomainControllerDef *co _("'ioeventfd' is only supported by virtio-scsi controller")); return -1; } + if (controller->iothread) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("'iothread' is only supported for virtio-scsi controller")); + return -1; + } } return 0; -- 2.13.6
On 01/06/2018 12:47 AM, John Ferlan wrote:
Let's make sure that non SCSI virtio-scsi isn't used for any type other than a virtio-scsi controller.
This includes removing the check from qemuCheckSCSIControllerIOThreads which is a very suble difference because although def->model was used in the original comparison and just @model is used in the new comparison, the comparison is the same.
This is because qemuDomainSetSCSIControllerModel doesn't change the def->model, thus we know that the resultant @model would result in either the same as input or if not set, whatever the default model is when def->model == -1. In this second case, virtio-scsi is the last default chosen.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 8 -------- src/qemu/qemu_domain.c | 5 +++++ 2 files changed, 5 insertions(+), 8 deletions(-)
ACK Michal
Move SCSI validation from qemu_command into qemu_domain. Rename/reorder the args in qemuCheckSCSIControllerIOThreads to match the caller 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 | 48 ++-------------------------------- src/qemu/qemu_domain.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 69 insertions(+), 47 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 732c720b0..45c244c15 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2583,44 +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->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 @@ -2663,12 +2625,9 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, 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"); @@ -2677,12 +2636,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; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f60c6540e..507bd2395 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4054,6 +4054,69 @@ 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, + int model, + const virDomainDef *def) +{ + 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, @@ -4082,8 +4145,11 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, ret = qemuDomainDeviceDefValidateControllerIDE(controller, def); break; - case VIR_DOMAIN_CONTROLLER_TYPE_FDC: case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: + ret = qemuDomainDeviceDefValidateControllerSCSI(controller, model, 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: -- 2.13.6
On 01/06/2018 12:47 AM, John Ferlan wrote:
Move SCSI validation from qemu_command into qemu_domain.
Rename/reorder the args in qemuCheckSCSIControllerIOThreads to match the caller 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 | 48 ++-------------------------------- src/qemu/qemu_domain.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 69 insertions(+), 47 deletions(-)
ACK Michal
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 | 47 ++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 45c244c15..07830f781 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2720,26 +2720,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 507bd2395..df265dc50 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4118,6 +4118,48 @@ qemuDomainDeviceDefValidateControllerSCSI(const virDomainControllerDef *controll static int +qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controller, + const virDomainDef *def) +{ + virDomainControllerModelPCI model = controller->model; + + /* skip pcie-root */ + if (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->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) @@ -4149,12 +4191,15 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, ret = qemuDomainDeviceDefValidateControllerSCSI(controller, model, def); 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 01/06/2018 12:47 AM, 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 | 47 ++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 21 deletions(-)
ACK Michal
From: Andrea Bolognani <abologna@redhat.com> We format the 'chassis' and 'port' properties on the QEMU command line later on, so we should make sure they've been set. Signed-off-by: Andrea Bolognani <abologna@redhat.com> Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 07830f781..d62531110 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2823,7 +2823,9 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: if (def->opts.pciopts.modelName - == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { + == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE || + def->opts.pciopts.chassis == -1 || + def->opts.pciopts.port == -1) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("autogenerated pcie-root-port options not set")); goto error; -- 2.13.6
On 01/06/2018 12:47 AM, John Ferlan wrote:
From: Andrea Bolognani <abologna@redhat.com>
We format the 'chassis' and 'port' properties on the QEMU command line later on, so we should make sure they've been set.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
ACK Michal
Shorten up a few characters and reference the pciopts pointer Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 118 ++++++++++++++++++++++++------------------------ 1 file changed, 60 insertions(+), 58 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d62531110..0175daee3 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; @@ -2718,24 +2719,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' " @@ -2750,26 +2753,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' " @@ -2784,28 +2787,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' " @@ -2822,24 +2825,23 @@ 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 || - def->opts.pciopts.chassis == -1 || - def->opts.pciopts.port == -1) { + pciopts->chassis == -1 || pciopts->port == -1) { 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' " @@ -2847,7 +2849,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", @@ -2855,7 +2857,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", @@ -2865,24 +2867,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' " @@ -2900,24 +2902,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' " @@ -2933,26 +2935,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' " @@ -2967,32 +2969,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); @@ -3005,17 +3007,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 01/06/2018 12:47 AM, 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 | 118 ++++++++++++++++++++++++------------------------ 1 file changed, 60 insertions(+), 58 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d62531110..0175daee3 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; @@ -2718,24 +2719,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 ||
Since you're touching this line, can you please move the == VIR_DOMAIN_.. onto the same line as pciopts->modelName? I know it's going to be longer than 80 characters (83 in fact), but: a) it's a soft limit, b) conditions written that way hurt my eyes more than long lines. EDIT: AH, you're removing the check in the next patch. Okay, no need to adjust anything then. ACK Michal
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 | 36 ++++++------------------------------ src/qemu/qemu_domain.c | 12 ++++++++++++ 2 files changed, 18 insertions(+), 30 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0175daee3..58f6bee3a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2723,9 +2723,7 @@ 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")); goto error; @@ -2757,9 +2755,7 @@ 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; @@ -2794,13 +2790,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, @@ -2825,9 +2814,7 @@ 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 || - pciopts->chassis == -1 || pciopts->port == -1) { + if (pciopts->chassis == -1 || pciopts->port == -1) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("autogenerated pcie-root-port options not set")); goto error; @@ -2871,12 +2858,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, @@ -2902,9 +2883,7 @@ 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", _("autogenerated pcie-switch-downstream-port " @@ -2939,9 +2918,7 @@ 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")); goto error; @@ -2976,8 +2953,7 @@ 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")); goto error; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index df265dc50..eb533f4b7 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4122,6 +4122,7 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle const virDomainDef *def) { virDomainControllerModelPCI model = controller->model; + const virDomainPCIControllerOpts *pciopts; /* skip pcie-root */ if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) @@ -4155,6 +4156,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 options not set"), + virDomainControllerModelPCITypeToString(controller->model)); + return -1; + } + } + return 0; } -- 2.13.6
On 01/06/2018 12:47 AM, 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.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 36 ++++++------------------------------ src/qemu/qemu_domain.c | 12 ++++++++++++ 2 files changed, 18 insertions(+), 30 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0175daee3..58f6bee3a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2723,9 +2723,7 @@ 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"));
I don't quite understand why break this check and move just one part of it into DefValidate and leave the other here. EDIT: Ah, you're doing that in one of the next patches. ACK then. Michal
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 58f6bee3a..4a5d37f5d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2720,6 +2720,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: @@ -2729,13 +2732,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, @@ -2761,13 +2757,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, goto error; } - 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, @@ -2790,13 +2779,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, @@ -2819,13 +2801,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, _("autogenerated pcie-root-port options not set")); goto error; } - 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 != @@ -2858,13 +2833,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, @@ -2891,13 +2859,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, @@ -2924,13 +2885,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, @@ -2963,13 +2917,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 eb533f4b7..082f1124c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4123,6 +4123,7 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle { virDomainControllerModelPCI model = controller->model; const virDomainPCIControllerOpts *pciopts; + const char *modelName = NULL; /* skip pcie-root */ if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) @@ -4165,6 +4166,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
On 01/06/2018 12:47 AM, John Ferlan wrote:
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(-)
ACK Michal
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 | 101 ----------------------------------- src/qemu/qemu_domain.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+), 101 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4a5d37f5d..0dbc73399 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2726,20 +2726,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 options 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 " @@ -2751,20 +2737,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 options not set")); - goto error; - } - - 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 " @@ -2779,14 +2751,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) " @@ -2796,21 +2760,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->chassis == -1 || pciopts->port == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pcie-root-port options not set")); - goto error; - } - 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)) { @@ -2833,14 +2782,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) " @@ -2851,22 +2792,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, "%s", - _("autogenerated pcie-switch-downstream-port " - "options not set")); - 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 " @@ -2879,20 +2804,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 options 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 " @@ -2907,22 +2818,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 options 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 082f1124c..8cbb29e40 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4135,6 +4135,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: @@ -4177,6 +4179,143 @@ 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 options 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 options 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->chassis == -1 || pciopts->port == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated pcie-root-port options not set")); + return -1; + } + + 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, "%s", + _("autogenerated pcie-switch-downstream-port " + "options not set")); + 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 options 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 options 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
On 01/06/2018 12:47 AM, John Ferlan wrote:
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 | 101 ----------------------------------- src/qemu/qemu_domain.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+), 101 deletions(-)
ACK Michal
Move the qemuCaps checks over to qemuDomainControllerDefValidatePCI. 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 | 70 +------------------------------------------ src/qemu/qemu_domain.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++-- tests/qemumemlocktest.c | 14 +++++++++ tests/qemuxml2xmltest.c | 5 +++- 4 files changed, 97 insertions(+), 72 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0dbc73399..7a138f921 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2726,23 +2726,11 @@ 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); @@ -2751,65 +2739,22 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, 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; - } - 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; - } - 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); @@ -2822,25 +2767,12 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, 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: diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8cbb29e40..55750615c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4119,7 +4119,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; @@ -4196,6 +4197,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: @@ -4213,6 +4221,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: @@ -4224,6 +4239,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: @@ -4242,6 +4264,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: @@ -4253,6 +4291,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: @@ -4271,6 +4316,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: @@ -4288,6 +4341,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: @@ -4309,6 +4369,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: @@ -4353,7 +4428,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 01/06/2018 12:47 AM, John Ferlan wrote:
Move the qemuCaps checks over to qemuDomainControllerDefValidatePCI.
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 | 70 +------------------------------------------ src/qemu/qemu_domain.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++-- tests/qemumemlocktest.c | 14 +++++++++ tests/qemuxml2xmltest.c | 5 +++- 4 files changed, 97 insertions(+), 72 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0dbc73399..7a138f921 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2726,23 +2726,11 @@ 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);
I'm worried that we cannot do this. As I say in one of of comments to previous patches - TOCTOU problem. What if somebody downgrades qemu between define & start times? In this light, I no longer think we can do 03/16, can we? Michal
On 01/28/2018 03:48 AM, Michal Privoznik wrote:
On 01/06/2018 12:47 AM, John Ferlan wrote:
Move the qemuCaps checks over to qemuDomainControllerDefValidatePCI.
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 | 70 +------------------------------------------ src/qemu/qemu_domain.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++-- tests/qemumemlocktest.c | 14 +++++++++ tests/qemuxml2xmltest.c | 5 +++- 4 files changed, 97 insertions(+), 72 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0dbc73399..7a138f921 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2726,23 +2726,11 @@ 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);
I'm worried that we cannot do this. As I say in one of of comments to previous patches - TOCTOU problem. What if somebody downgrades qemu between define & start times? In this light, I no longer think we can do 03/16, can we?
Michal
I assume you mean 2 & 3 then? Still TOCTOU would be true for any Validate vs. Run checks. Long ago, there was some review or comment made where there was a desire to move many of the domain validation checks from command line building into a separate phase so that the only thing that fails during command line building would be something that really caused the command line build to fail, not some configuration error. Doing that was tricky due to the domains disappearing problem. IIRC that's essentially why the Validation code was created. As for someone downgrading - well they're either going to fail on their next edit of their guest or they're going to fail when they try to run, right? It won't be a libvirt failure, it'll be a qemu failure. Still, if the run fails after a qemu downgrade because someone didn't validate that the guest that they changed to take advantage of some feature in the qemu that they now have downgraded from, then who's "problem" is that? <tongue-in-cheek> Ooohh - excellent qemu now supports X, let's upgrade and use X. So update qemu and alter the guest to use X. After using X it's determined, damn, I don't like X, so I'm going to downgrade to ensure X isn't used. Now if they don't update their guest to remove X as well, then the next start will fail in qemu because X doesn't exist. Of course they'll blame and curse libvirt, but maybe, just maybe, they'll recall they forgot to remove X from the guest. Who's fault is that? </tongue-in-cheek> Look if the desire is to just keep all the controller validation checks at run time, then fine - I can drop this series. That's fine. Personally, it felt better doing things during validate. Tks - John
On Fri, Jan 05, 2018 at 06:47:37PM -0500, John Ferlan wrote:
Move the qemuCaps checks over to qemuDomainControllerDefValidatePCI.
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 | 70 +------------------------------------------ src/qemu/qemu_domain.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++-- tests/qemumemlocktest.c | 14 +++++++++ tests/qemuxml2xmltest.c | 5 +++- 4 files changed, 97 insertions(+), 72 deletions(-)
ACK Jan
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 | 24 +++++++++++++++++++++++- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7a138f921..8c728c654 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2700,12 +2700,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 55750615c..bc077ed72 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4396,6 +4396,24 @@ 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->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) @@ -4432,8 +4450,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 01/06/2018 12:47 AM, 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 | 24 +++++++++++++++++++++++- 2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7a138f921..8c728c654 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2700,12 +2700,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);
Again, this falls into the same category of patches as the previous one. Michal
On Fri, Jan 05, 2018 at 06:47:38PM -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 | 24 +++++++++++++++++++++++- 2 files changed, 23 insertions(+), 7 deletions(-)
ACK Jan
From: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Use a switch statement instead of if-else-if statements. Move the command line building of the iothread attribute into the common path as the SCSI controller attributes are already validated. Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 45 +++++++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8c728c654..2523127cc 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; + int address_type = def->info.type; const virDomainPCIControllerOpts *pciopts; const char *modelName = NULL; @@ -2624,23 +2625,25 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, switch ((virDomainControllerModelSCSI) model) { case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: - if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { + switch ((virDomainDeviceAddressType) address_type) { + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW: virBufferAddLit(&buf, "virtio-scsi-ccw"); - if (def->iothread) - virBufferAsprintf(&buf, ",iothread=iothread%u", - def->iothread); - } else if (def->info.type == - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) { + break; + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390: virBufferAddLit(&buf, "virtio-scsi-s390"); - } else if (def->info.type == - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) { + break; + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO: virBufferAddLit(&buf, "virtio-scsi-device"); - } else { + break; + default: virBufferAddLit(&buf, "virtio-scsi-pci"); - if (def->iothread) - virBufferAsprintf(&buf, ",iothread=iothread%u", - def->iothread); } + + if (def->iothread) { + virBufferAsprintf(&buf, ",iothread=iothread%u", + def->iothread); + } + if (qemuBuildVirtioOptionsStr(&buf, def->virtio, qemuCaps) < 0) goto error; break; @@ -2668,18 +2671,20 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, break; case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: - if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + switch ((virDomainDeviceAddressType) address_type) { + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: virBufferAddLit(&buf, "virtio-serial-pci"); - } else if (def->info.type == - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { + break; + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW: virBufferAddLit(&buf, "virtio-serial-ccw"); - } else if (def->info.type == - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) { + break; + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390: virBufferAddLit(&buf, "virtio-serial-s390"); - } else if (def->info.type == - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) { + break; + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO: virBufferAddLit(&buf, "virtio-serial-device"); - } else { + break; + default: virBufferAddLit(&buf, "virtio-serial"); } virBufferAsprintf(&buf, ",id=%s", def->info.alias); -- 2.13.6
On 01/06/2018 12:47 AM, John Ferlan wrote:
From: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Use a switch statement instead of if-else-if statements. Move the command line building of the iothread attribute into the common path as the SCSI controller attributes are already validated.
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 45 +++++++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 20 deletions(-)
ACK Michal
ping? Should still apply with current top. I can resend if requested. Tks, John On 01/05/2018 06:47 PM, John Ferlan wrote:
v4: https://www.redhat.com/archives/libvir-list/2017-December/msg00401.html
Changes since v4:
Insert patch 1 to split the qemuDomainSetSCSIControllerModel as has been discussed during review.
Adjustments to the SCSI patches as a result.
Added patch 16 from Mark Hartmayer
Andrea Bolognani (1): qemu: Add missing checks for pcie-root-port options
John Ferlan (14): qemu: Split qemuDomainSetSCSIControllerModel qemu: Introduce qemuDomainDeviceDefValidateControllerAttributes qemu: Move and rename qemuBuildCheckSCSIControllerModel qemu: Move model set for qemuBuildControllerDevStr qemu: Adjust SCSI controller switch in qemuBuildControllerDevStr qemu: Add check for iothread attribute in validate controller 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
Marc Hartmayer (1): qemu: Use switch statement for address types in qemuBuildControllerDevStr
src/qemu/qemu_alias.c | 4 +- src/qemu/qemu_command.c | 452 +++++---------------------------------- src/qemu/qemu_domain.c | 474 ++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_domain_address.c | 74 ++----- src/qemu/qemu_domain_address.h | 6 +- tests/qemumemlocktest.c | 14 ++ tests/qemuxml2xmltest.c | 5 +- 7 files changed, 568 insertions(+), 461 deletions(-)
ping^2 Tks - John On 01/19/2018 01:27 PM, John Ferlan wrote:
ping?
Should still apply with current top. I can resend if requested.
Tks,
John
On 01/05/2018 06:47 PM, John Ferlan wrote:
v4: https://www.redhat.com/archives/libvir-list/2017-December/msg00401.html
Changes since v4:
Insert patch 1 to split the qemuDomainSetSCSIControllerModel as has been discussed during review.
Adjustments to the SCSI patches as a result.
Added patch 16 from Mark Hartmayer
Andrea Bolognani (1): qemu: Add missing checks for pcie-root-port options
John Ferlan (14): qemu: Split qemuDomainSetSCSIControllerModel qemu: Introduce qemuDomainDeviceDefValidateControllerAttributes qemu: Move and rename qemuBuildCheckSCSIControllerModel qemu: Move model set for qemuBuildControllerDevStr qemu: Adjust SCSI controller switch in qemuBuildControllerDevStr qemu: Add check for iothread attribute in validate controller 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
Marc Hartmayer (1): qemu: Use switch statement for address types in qemuBuildControllerDevStr
src/qemu/qemu_alias.c | 4 +- src/qemu/qemu_command.c | 452 +++++---------------------------------- src/qemu/qemu_domain.c | 474 ++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_domain_address.c | 74 ++----- src/qemu/qemu_domain_address.h | 6 +- tests/qemumemlocktest.c | 14 ++ tests/qemuxml2xmltest.c | 5 +- 7 files changed, 568 insertions(+), 461 deletions(-)
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (4)
-
John Ferlan -
Ján Tomko -
Michal Privoznik -
Peter Krempa