| Message ID | 155915010892.2061314.10485622810149098411.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
| Series | spapr_pci: Improve error message | expand |
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; > } > >
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; > > } > > > > >
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; > > > } > > > > > > > > >
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; }
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(-)