https://bugzilla.redhat.com/show_bug.cgi?id=1483816
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
src/qemu/qemu_domain.c | 229 +++++++++++++++++++++++++++++++------------------
1 file changed, 145 insertions(+), 84 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index a45a676520..13c2b557fb 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4274,7 +4274,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro
{
virDomainControllerModelPCI model = controller->model;
const virDomainPCIControllerOpts *pciopts;
- const char *modelName = NULL;
/* skip pcie-root */
if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
@@ -4312,24 +4311,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro
}
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;
- }
-
- modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
- if (!modelName) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unknown %s modelName value %d"),
- virDomainControllerModelPCITypeToString(controller->model),
- pciopts->modelName);
- return -1;
- }
- }
/* Second pass - now the model specific checks */
switch (model) {
@@ -4340,14 +4321,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro
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;
- }
-
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("the pci-bridge controller is not supported "
@@ -4364,14 +4337,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro
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;
- }
-
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("the pxb controller is not supported in this "
@@ -4382,14 +4347,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro
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;
- }
-
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("the dmi-to-pci-bridge (i82801b11-bridge) "
@@ -4406,15 +4363,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro
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;
- }
-
if ((pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) &&
!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IOH3420)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -4434,14 +4382,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro
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;
- }
-
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_X3130_UPSTREAM)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("the pcie-switch-upstream-port (x3130-upstream) "
@@ -4459,14 +4399,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro
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;
- }
-
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("The pcie-switch-downstream-port "
@@ -4484,14 +4416,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro
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;
- }
-
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB_PCIE)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("the pxb-pcie controller is not supported "
@@ -4512,14 +4436,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro
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 -1;
- }
-
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("the spapr-pci-host-bridge controller is not "
@@ -4566,6 +4482,151 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont,
return -1;
}
+ /* modelName */
+ switch ((virDomainControllerModelPCI) cont->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:
+ /* modelName should have been set automatically */
+ if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Option '%s' not set for '%s' controller"),
+ "modelName", model);
+ return -1;
+ }
+ break;
+
+ case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+ /* modelName must be set for pSeries guests, but it's an error
+ * for it to be set for any other guest */
+ if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE &&
+ qemuDomainIsPSeries(def)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Option '%s' not set for '%s' controller"),
+ "modelName", model);
+ return -1;
+ }
+ if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE &&
+ !qemuDomainIsPSeries(def)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Option '%s' is not valid for '%s' controller"),
+ "modelName", model);
+ return -1;
+ }
+ break;
+
+ case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
+ if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Option '%s' is not valid for '%s' controller"),
+ "modelName", model);
+ return -1;
+ }
+ break;
+
+ case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
+ case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
+ default:
+ virReportEnumRangeError(virDomainControllerModelPCI, cont->model);
+ return -1;
+ }
+
+ /* modelName (cont'd) */
+ switch ((virDomainControllerModelPCI) cont->model) {
+ case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+ if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE &&
+ pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Model name '%s' is not valid for '%s' controller"),
+ modelName, "pci-root");
+ return -1;
+ }
+ break;
+
+ case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
+ if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Model name '%s' is not valid for '%s' controller"),
+ modelName, "pci-bridge");
+ 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,
+ _("Model name '%s' is not valid for '%s' controller"),
+ modelName, "dmi-to-pci-bridge");
+ return -1;
+ }
+ break;
+
+ case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
+ if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420 &&
+ pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Model name '%s' is not valid for '%s' controller"),
+ modelName, "pcie-root-port");
+ 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,
+ _("Model name '%s' is not valid for '%s' controller"),
+ modelName, "pcie-switch-upstream-port");
+ return -1;
+ }
+ break;
+
+ case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
+ if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Model name '%s' is not valid for '%s' controller"),
+ modelName, "pci-switch-downstream-port");
+ return -1;
+ }
+ break;
+
+ case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
+ if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Model name '%s' is not valid for '%s' controller"),
+ modelName, "pci-expander-bus");
+ return -1;
+ }
+ break;
+
+ case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
+ if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Model name '%s' is not valid for '%s' controller"),
+ modelName, "pcie-expander-bus");
+ return -1;
+ }
+ break;
+
+ case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
+ if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Model name '%s' is not valid for '%s' controller"),
+ modelName, "pcie-root");
+ return -1;
+ }
+ break;
+
+ case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
+ case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
+ default:
+ virReportEnumRangeError(virDomainControllerModelPCI, cont->model);
+ return -1;
+ }
+
return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps);
}
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 03/02/2018 10:13 AM, Andrea Bolognani wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1483816
>
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
> src/qemu/qemu_domain.c | 229 +++++++++++++++++++++++++++++++------------------
> 1 file changed, 145 insertions(+), 84 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index a45a676520..13c2b557fb 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4274,7 +4274,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro
> {
> virDomainControllerModelPCI model = controller->model;
> const virDomainPCIControllerOpts *pciopts;
> - const char *modelName = NULL;
>
> /* skip pcie-root */
> if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
> @@ -4312,24 +4311,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro
> }
>
> 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;
> - }
> -
> - modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
> - if (!modelName) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("unknown %s modelName value %d"),
> - virDomainControllerModelPCITypeToString(controller->model),
> - pciopts->modelName);
> - return -1;
> - }
> - }
>
> /* Second pass - now the model specific checks */
> switch (model) {
> @@ -4340,14 +4321,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro
> 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;
> - }
> -
> if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("the pci-bridge controller is not supported "
> @@ -4364,14 +4337,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro
> 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;
> - }
> -
> if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("the pxb controller is not supported in this "
> @@ -4382,14 +4347,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro
> 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;
> - }
> -
> if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("the dmi-to-pci-bridge (i82801b11-bridge) "
> @@ -4406,15 +4363,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro
> 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;
> - }
> -
> if ((pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) &&
> !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IOH3420)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> @@ -4434,14 +4382,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro
> 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;
> - }
> -
> if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_X3130_UPSTREAM)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("the pcie-switch-upstream-port (x3130-upstream) "
> @@ -4459,14 +4399,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro
> 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;
> - }
> -
> if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("The pcie-switch-downstream-port "
> @@ -4484,14 +4416,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro
> 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;
> - }
> -
> if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB_PCIE)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("the pxb-pcie controller is not supported "
> @@ -4512,14 +4436,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef *contro
> 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 -1;
> - }
> -
> if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("the spapr-pci-host-bridge controller is not "
> @@ -4566,6 +4482,151 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont,
> return -1;
> }
>
> + /* modelName */
> + switch ((virDomainControllerModelPCI) cont->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:
> + /* modelName should have been set automatically */
> + if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Option '%s' not set for '%s' controller"),
> + "modelName", model);
I'm undecided if it's useful to have a special function for this (since
it's going to be used so many times), or if it's just as efficient to
spell it out every time (since the compiler will combine all the uses of
the same literal string into a single instance of the string in memory /
in the translations).
maybe virReportControllerMissingOption(model, option) - less characters
to type, and simpler to change the wording for all occurrences if we
decide to, but maybe less informative when reading the code.).
As an example of how we might decide to change the exact message - I
think it might be better if it was "Required option '%s' not set for
'%s' controller".
Hmm. Although, I see from at least one example that there will need to
be variations of the message - in the case of targetIndex, for example,
it is only required if the pci-root controller is an
spapr-pci-host-bridge - the error in that case really should have that
info (or maybe the index of the controller would be even better).
> + return -1;
> + }
> + break;
> +
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> + /* modelName must be set for pSeries guests, but it's an error
> + * for it to be set for any other guest */
> + if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE &&
> + qemuDomainIsPSeries(def)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Option '%s' not set for '%s' controller"),
> + "modelName", model);
> + return -1;
> + }
> + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE &&
> + !qemuDomainIsPSeries(def)) {
micro-optimization - you could structure this as:
if (qemuDomainIsPSeries(def) {
if (pciopts->modelName == .....NONE) {
BigFatGreekError();
return -1;
}
} else {
if (pciopts->modelName != ....NONE) {
BigFatGreekError();
return -1;
}
}
so that qemuDomainIsPSeries() was only called once.
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Option '%s' is not valid for '%s' controller"),
> + "modelName", model);
Maybe a helper function for this error too.
(virReportInvalidControllerModelName(model, modelName)?) Again, I'm
undecided if having a separate function (actually a #defined macro so
that function/line number info would be correct in the log) would be
useful for maintenance, or just extra work for nothing.
> + return -1;
> + }
> + break;
> +
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Option '%s' is not valid for '%s' controller"),
> + "modelName", model);
> + return -1;
> + }
> + break;
> +
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
> + default:
> + virReportEnumRangeError(virDomainControllerModelPCI, cont->model);
> + return -1;
> + }
> +
> + /* modelName (cont'd) */
> + switch ((virDomainControllerModelPCI) cont->model) {
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE &&
> + pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Model name '%s' is not valid for '%s' controller"),
> + modelName, "pci-root");
Why hardcode the model instead of just using the string that has already
been set with that value?
> + return -1;
> + }
> + break;
> +
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
> + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Model name '%s' is not valid for '%s' controller"),
> + modelName, "pci-bridge");
same. etc etc.
> + 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,
> + _("Model name '%s' is not valid for '%s' controller"),
> + modelName, "dmi-to-pci-bridge");
> + return -1;
> + }
> + break;
> +
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
> + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420 &&
> + pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Model name '%s' is not valid for '%s' controller"),
> + modelName, "pcie-root-port");
> + 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,
> + _("Model name '%s' is not valid for '%s' controller"),
> + modelName, "pcie-switch-upstream-port");
> + return -1;
> + }
> + break;
> +
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
> + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Model name '%s' is not valid for '%s' controller"),
> + modelName, "pci-switch-downstream-port");
> + return -1;
> + }
> + break;
> +
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
> + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Model name '%s' is not valid for '%s' controller"),
> + modelName, "pci-expander-bus");
> + return -1;
> + }
> + break;
> +
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
> + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Model name '%s' is not valid for '%s' controller"),
> + modelName, "pcie-expander-bus");
> + return -1;
> + }
> + break;
> +
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Model name '%s' is not valid for '%s' controller"),
> + modelName, "pcie-root");
> + return -1;
> + }
> + break;
> +
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
> + default:
> + virReportEnumRangeError(virDomainControllerModelPCI, cont->model);
> + return -1;
> + }
> +
> return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps);
> }
>
I'm not sure I see the advantage of grouping the validation for a single
attribute on several different controller models together, rather than
the current code's grouping validation for all attributes on a single
controller together. Potato-potahto. It does mean that if you're hand
tracing the code you need to go through the entire function rather than
just one section. On the other hand, I guess it does mean that many
instances of "attribute X isn't valid for Y controller" can be combined.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, 2018-03-02 at 16:16 -0500, Laine Stump wrote:
[...]
> > + /* modelName */
> > + switch ((virDomainControllerModelPCI) cont->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:
> > + /* modelName should have been set automatically */
> > + if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Option '%s' not set for '%s' controller"),
> > + "modelName", model);
>
> I'm undecided if it's useful to have a special function for this (since
> it's going to be used so many times), or if it's just as efficient to
> spell it out every time (since the compiler will combine all the uses of
> the same literal string into a single instance of the string in memory /
> in the translations).
>
> maybe virReportControllerMissingOption(model, option) - less characters
> to type, and simpler to change the wording for all occurrences if we
> decide to, but maybe less informative when reading the code.).
>
> As an example of how we might decide to change the exact message - I
> think it might be better if it was "Required option '%s' not set for
> '%s' controller".
>
> Hmm. Although, I see from at least one example that there will need to
> be variations of the message - in the case of targetIndex, for example,
> it is only required if the pci-root controller is an
> spapr-pci-host-bridge - the error in that case really should have that
> info (or maybe the index of the controller would be even better).
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("Option '%s' is not valid for '%s' controller"),
> > + "modelName", model);
>
> Maybe a helper function for this error too.
> (virReportInvalidControllerModelName(model, modelName)?) Again, I'm
> undecided if having a separate function (actually a #defined macro so
> that function/line number info would be correct in the log) would be
> useful for maintenance, or just extra work for nothing.
I've given it a try and it makes things quite a bit nicer. Showing
the controller index is also very useful, as some controllers can
show up multiple times in a guest.
A couple error messages might have gotten slightly less informative
as a result, but I'd say it's acceptable as long as there's enough
context to help the user look it up in the documentation.
[...]
> > + /* modelName (cont'd) */
> > + switch ((virDomainControllerModelPCI) cont->model) {
> > + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> > + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE &&
> > + pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("Model name '%s' is not valid for '%s' controller"),
> > + modelName, "pci-root");
>
> Why hardcode the model instead of just using the string that has already
> been set with that value?
Just a temporary brain fart: I've gotten it right in subsequent
patches, and I've now fixed it here as well.
> I'm not sure I see the advantage of grouping the validation for a single
> attribute on several different controller models together, rather than
> the current code's grouping validation for all attributes on a single
> controller together. Potato-potahto. It does mean that if you're hand
> tracing the code you need to go through the entire function rather than
> just one section. On the other hand, I guess it does mean that many
> instances of "attribute X isn't valid for Y controller" can be combined.
Yeah, structuring the code this way allows to provide better error
messages (eg. spelling out which specific option is missing) without
resulting in an endless amount of repetition; it also makes adding
checks for all options when introducing a new controller model more
difficult to forget, though unfortunately it doesn't help when
adding a new option instead.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.