On Thu, May 30, 2019 at 09:40:27AM +0200, Greg Kurz wrote:
> 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:
Duh, sorry. I was misreading that as appending to the error message
itself, rather than separate hints. Applied.
>
> /*
> * 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;
> > > }
> > >
> > >
> >
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
