On Fri, 2018-03-02 at 16:16 -0500, Laine Stump wrote:
[...]
> > +    /* 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.

I've given it a try and it makes things quite a bit nicer. Showing
the controller index is also very useful, as some controllers can
show up multiple times in a guest.

A couple error messages might have gotten slightly less informative
as a result, but I'd say it's acceptable as long as there's enough
context to help the user look it up in the documentation.

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

Just a temporary brain fart: I've gotten it right in subsequent
patches, and I've now fixed it here as well.

> 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.

Yeah, structuring the code this way allows to provide better error
messages (eg. spelling out which specific option is missing) without
resulting in an endless amount of repetition; it also makes adding
checks for all options when introducing a new controller model more
difficult to forget, though unfortunately it doesn't help when
adding a new option instead.

-- 
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