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

Reply via email to