spapr_pci: Improve error message

[Qemu-devel] [PATCH] spapr_pci: Improve error message

Posted by Greg Kurz 4 years, 10 months ago

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 <groug@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");
         return;
     }

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

Posted by David Gibson 4 years, 10 months ago

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

>          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

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

Posted by Greg Kurz 4 years, 10 months ago

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

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

Posted by David Gibson 4 years, 10 months ago

On Thu, May 30, 2019 at 09:40:27AM +0200, Greg Kurz wrote:
> On Thu, 30 May 2019 10:40:49 +1000
> David Gibson <david@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 <groug@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