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@xxxxxxxxxx> > --- > 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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
