On Mon, 2018-02-19 at 11:52 +0000, Daniel P. Berrangé wrote:
+qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controller, + const virDomainDef *def) { + const virDomainPCIControllerOpts *pciopts = &controller->opts.pciopts; + const char *model = virDomainControllerModelPCITypeToString(controller->model); + const char *modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); + + if (!model) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown virDomainControllerModelPCI value: %d"),
Including type names in error messages is not too nice as a user facing error - better to use plain english "Unexpected PCI controller model")
But these are marked as internal errors, eg. They Should Never Happen™, and if they ever do it's useful for developers to have more detailed information. Users can't really do anything but report the issue, after all.
+ case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + /* pci-root controllers for pSeries guests can have any index, but + * all other pci-root and pcie-root controller must have index 0 */ + if (controller->idx != 0 && + !(qemuDomainIsPSeries(def) && + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Index for '%s' controller must be 0"), + model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
Any time there is a _LAST element we should have an error report too
virReportError(VIR_ERR_INTERNAL_ERROR, _("Unexpected controller model %d"), controller->model); return -1;
It should also have an 'default:' next to the LAST. Both are things that should never occur unless we've screwed up code elsewherein libvirt, but we want to be robust about catching such screw ups. This is particularly important for controller models, as we have many different controller type specific enums all stored in the same struct field
Okay, I'll address that, but the point above about what error message to show in such scenarios still applies IMHO. -- Andrea Bolognani / Red Hat / Virtualization