Re: [libvirt] [PATCH v5 03/12] qemu: Validate PCI controller options (modelName)

> + /* 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.

> + /* 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?

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.