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