On Thu, 30 May 2019 10:40:49 +1000 David Gibson <da...@gibson.dropbear.id.au> wrote:
> On Wed, May 29, 2019 at 07:15:09PM +0200, Greg Kurz wrote: > > Every PHB must have a unique index. This is checked at realize but when > > a duplicate index is detected, an error message mentioning BUIDs is > > printed. This doesn't help much, especially since BUID is an internal > > concept that is no longer exposed to the user. > > > > Fix the message to mention the index property instead of BUID. As a bonus > > print a list of indexes already in use. > > > > Signed-off-by: Greg Kurz <gr...@kaod.org> > > --- > > hw/ppc/spapr_pci.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > > index 97961b012859..fb8c54f4d90f 100644 > > --- a/hw/ppc/spapr_pci.c > > +++ b/hw/ppc/spapr_pci.c > > @@ -1699,7 +1699,14 @@ static void spapr_phb_realize(DeviceState *dev, > > Error **errp) > > } > > > > if (spapr_pci_find_phb(spapr, sphb->buid)) { > > - error_setg(errp, "PCI host bridges must have unique BUIDs"); > > + SpaprPhbState *s; > > + > > + error_setg(errp, "PCI host bridges must have unique indexes"); > > + error_append_hint(errp, "The following indexes are already in > > use:"); > > + QLIST_FOREACH(s, &spapr->phbs, list) { > > + error_append_hint(errp, " %d", s->index); > > + } > > + error_append_hint(errp, "\nTry another value for the index > > property\n"); > > I like the idea, but I think newlines in error messages are frowned > upon. You certainly don't need the trailing one. > newlines are definitely not welcome in strings passed to error_report() or error_setg(), but they are okay in hints and the trailing one is actually required: /* * Append a printf-style human-readable explanation to an existing error. * If the error is later reported to a human user with * error_report_err() or warn_report_err(), the hints will be shown, * too. If it's reported via QMP, the hints will be ignored. * Intended use is adding helpful hints on the human user interface, * e.g. a list of valid values. It's not for clarifying a confusing * error message. * @errp may be NULL, but not &error_fatal or &error_abort. * Trivially the case if you call it only after error_setg() or * error_propagate(). * May be called multiple times. The resulting hint should end with a * newline. */ void error_append_hint(Error **errp, const char *fmt, ...) GCC_FMT_ATTR(2, 3); Cc'ing Markus for insights. > > return; > > } > > > > >
pgpxhr2mr9aZN.pgp
Description: OpenPGP digital signature