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

[David Gibson]

On Thu, May 30, 2019 at 09:40:27AM +0200, Greg Kurz wrote:
> 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:

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