Re: [Qemu-ppc] [PATCH] spapr_pci: Improve error message

[Greg Kurz]

On Thu, 30 May 2019 10:40:49 +1000
David Gibson <address@hidden> 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 <address@hidden>
> > ---
> >  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;
> >      }
> >  
> >   
>

pgpunADXwYdrH.pgp
Description: OpenPGP digital signature