| Message ID | 159186660024.48605.6756496231687601694.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
| Series | spapr: Improve error reporting in spapr_caps.c | expand |
11.06.2020 12:10, Greg Kurz wrote: > We have a dedicated error API for hints. Use it instead of embedding > the hint in the error message, as recommanded in the "qapi/error.h" > header file. > > Since spapr_caps_apply() passes &error_fatal, all functions must > also call the ERRP_AUTO_PROPAGATE() macro for error_append_hint() > to be functional. > > While here, add some missing braces around one line statements that > are part of the patch context. Also have cap_fwnmi_apply(), which > already uses error_append_hint() to call ERRP_AUTO_PROPAGATE() as > well. > > Signed-off-by: Greg Kurz<groug@kaod.org> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
On 11/06/2020 11:10, Greg Kurz wrote: > We have a dedicated error API for hints. Use it instead of embedding > the hint in the error message, as recommanded in the "qapi/error.h" > header file. > > Since spapr_caps_apply() passes &error_fatal, all functions must > also call the ERRP_AUTO_PROPAGATE() macro for error_append_hint() > to be functional. > > While here, add some missing braces around one line statements that > are part of the patch context. Also have cap_fwnmi_apply(), which > already uses error_append_hint() to call ERRP_AUTO_PROPAGATE() as > well. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/ppc/spapr_caps.c | 95 +++++++++++++++++++++++++++++---------------------- > 1 file changed, 54 insertions(+), 41 deletions(-) > > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c > index efdc0dbbcfc0..2cb7ba8f005a 100644 > --- a/hw/ppc/spapr_caps.c > +++ b/hw/ppc/spapr_caps.c ... > @@ -248,6 +249,7 @@ SpaprCapPossible cap_cfpc_possible = { > static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, > Error **errp) > { > + ERRP_AUTO_PROPAGATE(); > Error *local_err = NULL; I think you should rename it, something like "local_warn" to not be confused with the _auto_errp_prop.local_err... or don't use ERRP_AUTO_PROPAGE(), use the local_err instead and move the warning inside the braces of the if. Same comment for cap_safe_bounds_check_apply() and cap_safe_indirect_branch_apply() > uint8_t kvm_val = kvmppc_get_cap_safe_cache(); > > @@ -258,13 +260,14 @@ static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, > cap_cfpc_possible.vals[val]); > } else if (kvm_enabled() && (val > kvm_val)) { > error_setg(errp, > - "Requested safe cache capability level not supported by kvm," > - " try appending -machine cap-cfpc=%s", > - cap_cfpc_possible.vals[kvm_val]); > + "Requested safe cache capability level not supported by KVM"); > + error_append_hint(errp, "Try appending -machine cap-cfpc=%s\n", > + cap_cfpc_possible.vals[kvm_val]); > } > > - if (local_err != NULL) > + if (local_err != NULL) { > warn_report_err(local_err); > + } > } > > SpaprCapPossible cap_sbbc_possible = { Thanks, Laurent
On Thu, 11 Jun 2020 11:50:57 +0200 Laurent Vivier <lvivier@redhat.com> wrote: > On 11/06/2020 11:10, Greg Kurz wrote: > > We have a dedicated error API for hints. Use it instead of embedding > > the hint in the error message, as recommanded in the "qapi/error.h" > > header file. > > > > Since spapr_caps_apply() passes &error_fatal, all functions must > > also call the ERRP_AUTO_PROPAGATE() macro for error_append_hint() > > to be functional. > > > > While here, add some missing braces around one line statements that > > are part of the patch context. Also have cap_fwnmi_apply(), which > > already uses error_append_hint() to call ERRP_AUTO_PROPAGATE() as > > well. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/ppc/spapr_caps.c | 95 +++++++++++++++++++++++++++++---------------------- > > 1 file changed, 54 insertions(+), 41 deletions(-) > > > > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c > > index efdc0dbbcfc0..2cb7ba8f005a 100644 > > --- a/hw/ppc/spapr_caps.c > > +++ b/hw/ppc/spapr_caps.c > ... > > @@ -248,6 +249,7 @@ SpaprCapPossible cap_cfpc_possible = { > > static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, > > Error **errp) > > { > > + ERRP_AUTO_PROPAGATE(); > > Error *local_err = NULL; > > I think you should rename it, something like "local_warn" to not be > confused with the _auto_errp_prop.local_err... > > or don't use ERRP_AUTO_PROPAGE(), use the local_err instead and move the > warning inside the braces of the if. > > Same comment for cap_safe_bounds_check_apply() and > cap_safe_indirect_branch_apply() > Hmm... local_err isn't useful actually. It looks like we just want to call warn_report() directly instead of error_setg(&local_err) and warn_report_err(local_err). I'll post a v3. > > uint8_t kvm_val = kvmppc_get_cap_safe_cache(); > > > > @@ -258,13 +260,14 @@ static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, > > cap_cfpc_possible.vals[val]); > > } else if (kvm_enabled() && (val > kvm_val)) { > > error_setg(errp, > > - "Requested safe cache capability level not supported by kvm," > > - " try appending -machine cap-cfpc=%s", > > - cap_cfpc_possible.vals[kvm_val]); > > + "Requested safe cache capability level not supported by KVM"); > > + error_append_hint(errp, "Try appending -machine cap-cfpc=%s\n", > > + cap_cfpc_possible.vals[kvm_val]); > > } > > > > - if (local_err != NULL) > > + if (local_err != NULL) { > > warn_report_err(local_err); > > + } > > } > > > > SpaprCapPossible cap_sbbc_possible = { > > Thanks, > Laurent >
11.06.2020 13:13, Greg Kurz wrote: > On Thu, 11 Jun 2020 11:50:57 +0200 > Laurent Vivier <lvivier@redhat.com> wrote: > >> On 11/06/2020 11:10, Greg Kurz wrote: >>> We have a dedicated error API for hints. Use it instead of embedding >>> the hint in the error message, as recommanded in the "qapi/error.h" >>> header file. >>> >>> Since spapr_caps_apply() passes &error_fatal, all functions must >>> also call the ERRP_AUTO_PROPAGATE() macro for error_append_hint() >>> to be functional. >>> >>> While here, add some missing braces around one line statements that >>> are part of the patch context. Also have cap_fwnmi_apply(), which >>> already uses error_append_hint() to call ERRP_AUTO_PROPAGATE() as >>> well. >>> >>> Signed-off-by: Greg Kurz <groug@kaod.org> >>> --- >>> hw/ppc/spapr_caps.c | 95 +++++++++++++++++++++++++++++---------------------- >>> 1 file changed, 54 insertions(+), 41 deletions(-) >>> >>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c >>> index efdc0dbbcfc0..2cb7ba8f005a 100644 >>> --- a/hw/ppc/spapr_caps.c >>> +++ b/hw/ppc/spapr_caps.c >> ... >>> @@ -248,6 +249,7 @@ SpaprCapPossible cap_cfpc_possible = { >>> static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, >>> Error **errp) >>> { >>> + ERRP_AUTO_PROPAGATE(); >>> Error *local_err = NULL; >> >> I think you should rename it, something like "local_warn" to not be >> confused with the _auto_errp_prop.local_err... >> >> or don't use ERRP_AUTO_PROPAGE(), use the local_err instead and move the >> warning inside the braces of the if. >> >> Same comment for cap_safe_bounds_check_apply() and >> cap_safe_indirect_branch_apply() >> > > Hmm... local_err isn't useful actually. It looks like we just want > to call warn_report() directly instead of error_setg(&local_err) > and warn_report_err(local_err). I'll post a v3. something like this I think: --- a/hw/ppc/spapr_caps.c +++ b/hw/ppc/spapr_caps.c @@ -250,24 +250,23 @@ static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) { ERRP_AUTO_PROPAGATE(); - Error *local_err = NULL; uint8_t kvm_val = kvmppc_get_cap_safe_cache(); if (tcg_enabled() && val) { /* TCG only supports broken, allow other values and print a warning */ - error_setg(&local_err, + error_setg(errp, "TCG doesn't support requested feature, cap-cfpc=%s", cap_cfpc_possible.vals[val]); + if (*errp) { + warn_report_err(*errp); + *errp = NULL; + } } else if (kvm_enabled() && (val > kvm_val)) { error_setg(errp, "Requested safe cache capability level not supported by KVM"); error_append_hint(errp, "Try appending -machine cap-cfpc=%s\n", cap_cfpc_possible.vals[kvm_val]); } - - if (local_err != NULL) { - warn_report_err(local_err); - } } Or, we need to implement warn_report_errp() function, as I proposed in earlier version of auto-propagation series. ===== side idea: what if we make Error to be some kind of enum of pointer-to-pointer and pointer-to-function? Than, instead of passing pointers to error_abort and error_fatal as special casing, we'll pass pointers to functions, which do appropriate handling of error. And we'll be able to pass warn_report function. Not about this patch set, but seems interesting, isn't it?
11.06.2020 13:21, Vladimir Sementsov-Ogievskiy wrote: > 11.06.2020 13:13, Greg Kurz wrote: >> On Thu, 11 Jun 2020 11:50:57 +0200 >> Laurent Vivier <lvivier@redhat.com> wrote: >> >>> On 11/06/2020 11:10, Greg Kurz wrote: >>>> We have a dedicated error API for hints. Use it instead of embedding >>>> the hint in the error message, as recommanded in the "qapi/error.h" >>>> header file. >>>> >>>> Since spapr_caps_apply() passes &error_fatal, all functions must >>>> also call the ERRP_AUTO_PROPAGATE() macro for error_append_hint() >>>> to be functional. >>>> >>>> While here, add some missing braces around one line statements that >>>> are part of the patch context. Also have cap_fwnmi_apply(), which >>>> already uses error_append_hint() to call ERRP_AUTO_PROPAGATE() as >>>> well. >>>> >>>> Signed-off-by: Greg Kurz <groug@kaod.org> >>>> --- >>>> hw/ppc/spapr_caps.c | 95 +++++++++++++++++++++++++++++---------------------- >>>> 1 file changed, 54 insertions(+), 41 deletions(-) >>>> >>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c >>>> index efdc0dbbcfc0..2cb7ba8f005a 100644 >>>> --- a/hw/ppc/spapr_caps.c >>>> +++ b/hw/ppc/spapr_caps.c >>> ... >>>> @@ -248,6 +249,7 @@ SpaprCapPossible cap_cfpc_possible = { >>>> static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, >>>> Error **errp) >>>> { >>>> + ERRP_AUTO_PROPAGATE(); >>>> Error *local_err = NULL; >>> >>> I think you should rename it, something like "local_warn" to not be >>> confused with the _auto_errp_prop.local_err... >>> >>> or don't use ERRP_AUTO_PROPAGE(), use the local_err instead and move the >>> warning inside the braces of the if. >>> >>> Same comment for cap_safe_bounds_check_apply() and >>> cap_safe_indirect_branch_apply() >>> >> >> Hmm... local_err isn't useful actually. It looks like we just want >> to call warn_report() directly instead of error_setg(&local_err) >> and warn_report_err(local_err). I'll post a v3. > > something like this I think: > > --- a/hw/ppc/spapr_caps.c > +++ b/hw/ppc/spapr_caps.c > @@ -250,24 +250,23 @@ static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, > Error **errp) > { > ERRP_AUTO_PROPAGATE(); > - Error *local_err = NULL; > uint8_t kvm_val = kvmppc_get_cap_safe_cache(); > > if (tcg_enabled() && val) { > /* TCG only supports broken, allow other values and print a warning */ > - error_setg(&local_err, > + error_setg(errp, > "TCG doesn't support requested feature, cap-cfpc=%s", > cap_cfpc_possible.vals[val]); > + if (*errp) { > + warn_report_err(*errp); > + *errp = NULL; > + } > } else if (kvm_enabled() && (val > kvm_val)) { > error_setg(errp, > "Requested safe cache capability level not supported by KVM"); > error_append_hint(errp, "Try appending -machine cap-cfpc=%s\n", > cap_cfpc_possible.vals[kvm_val]); > } > - > - if (local_err != NULL) { > - warn_report_err(local_err); > - } > } > > > Or, we need to implement warn_report_errp() function, as I proposed in earlier version of auto-propagation series. > > ===== > > side idea: what if we make Error to be some kind of enum of pointer-to-pointer and pointer-to-function? union I mean of course > > Than, instead of passing pointers to error_abort and error_fatal as special casing, we'll pass pointers to functions, > which do appropriate handling of error. And we'll be able to pass warn_report function. Not about this patch set, > but seems interesting, isn't it? >
On Thu, 11 Jun 2020 13:21:51 +0300 Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > 11.06.2020 13:13, Greg Kurz wrote: > > On Thu, 11 Jun 2020 11:50:57 +0200 > > Laurent Vivier <lvivier@redhat.com> wrote: > > > >> On 11/06/2020 11:10, Greg Kurz wrote: > >>> We have a dedicated error API for hints. Use it instead of embedding > >>> the hint in the error message, as recommanded in the "qapi/error.h" > >>> header file. > >>> > >>> Since spapr_caps_apply() passes &error_fatal, all functions must > >>> also call the ERRP_AUTO_PROPAGATE() macro for error_append_hint() > >>> to be functional. > >>> > >>> While here, add some missing braces around one line statements that > >>> are part of the patch context. Also have cap_fwnmi_apply(), which > >>> already uses error_append_hint() to call ERRP_AUTO_PROPAGATE() as > >>> well. > >>> > >>> Signed-off-by: Greg Kurz <groug@kaod.org> > >>> --- > >>> hw/ppc/spapr_caps.c | 95 +++++++++++++++++++++++++++++---------------------- > >>> 1 file changed, 54 insertions(+), 41 deletions(-) > >>> > >>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c > >>> index efdc0dbbcfc0..2cb7ba8f005a 100644 > >>> --- a/hw/ppc/spapr_caps.c > >>> +++ b/hw/ppc/spapr_caps.c > >> ... > >>> @@ -248,6 +249,7 @@ SpaprCapPossible cap_cfpc_possible = { > >>> static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, > >>> Error **errp) > >>> { > >>> + ERRP_AUTO_PROPAGATE(); > >>> Error *local_err = NULL; > >> > >> I think you should rename it, something like "local_warn" to not be > >> confused with the _auto_errp_prop.local_err... > >> > >> or don't use ERRP_AUTO_PROPAGE(), use the local_err instead and move the > >> warning inside the braces of the if. > >> > >> Same comment for cap_safe_bounds_check_apply() and > >> cap_safe_indirect_branch_apply() > >> > > > > Hmm... local_err isn't useful actually. It looks like we just want > > to call warn_report() directly instead of error_setg(&local_err) > > and warn_report_err(local_err). I'll post a v3. > > something like this I think: > Not even that... we have one path (KVM) that directly uses errp and the other path (TCG) that does: Error *local_err = NULL; error_setg(&local_err, ...); if (local_err) { warn_report_err(local_err); } It really looks like we just want to call warn_report(). > --- a/hw/ppc/spapr_caps.c > +++ b/hw/ppc/spapr_caps.c > @@ -250,24 +250,23 @@ static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, > Error **errp) > { > ERRP_AUTO_PROPAGATE(); > - Error *local_err = NULL; > uint8_t kvm_val = kvmppc_get_cap_safe_cache(); > > if (tcg_enabled() && val) { > /* TCG only supports broken, allow other values and print a warning */ > - error_setg(&local_err, > + error_setg(errp, > "TCG doesn't support requested feature, cap-cfpc=%s", > cap_cfpc_possible.vals[val]); > + if (*errp) { > + warn_report_err(*errp); > + *errp = NULL; > + } > } else if (kvm_enabled() && (val > kvm_val)) { > error_setg(errp, > "Requested safe cache capability level not supported by KVM"); > error_append_hint(errp, "Try appending -machine cap-cfpc=%s\n", > cap_cfpc_possible.vals[kvm_val]); > } > - > - if (local_err != NULL) { > - warn_report_err(local_err); > - } > } > > > Or, we need to implement warn_report_errp() function, as I proposed in earlier version of auto-propagation series. > > ===== > > side idea: what if we make Error to be some kind of enum of pointer-to-pointer and pointer-to-function? > > Than, instead of passing pointers to error_abort and error_fatal as special casing, we'll pass pointers to functions, > which do appropriate handling of error. And we'll be able to pass warn_report function. Not about this patch set, > but seems interesting, isn't it? >
11.06.2020 13:21, Vladimir Sementsov-Ogievskiy wrote: > 11.06.2020 13:13, Greg Kurz wrote: >> On Thu, 11 Jun 2020 11:50:57 +0200 >> Laurent Vivier <lvivier@redhat.com> wrote: >> >>> On 11/06/2020 11:10, Greg Kurz wrote: >>>> We have a dedicated error API for hints. Use it instead of embedding >>>> the hint in the error message, as recommanded in the "qapi/error.h" >>>> header file. >>>> >>>> Since spapr_caps_apply() passes &error_fatal, all functions must >>>> also call the ERRP_AUTO_PROPAGATE() macro for error_append_hint() >>>> to be functional. >>>> >>>> While here, add some missing braces around one line statements that >>>> are part of the patch context. Also have cap_fwnmi_apply(), which >>>> already uses error_append_hint() to call ERRP_AUTO_PROPAGATE() as >>>> well. >>>> >>>> Signed-off-by: Greg Kurz <groug@kaod.org> >>>> --- >>>> hw/ppc/spapr_caps.c | 95 +++++++++++++++++++++++++++++---------------------- >>>> 1 file changed, 54 insertions(+), 41 deletions(-) >>>> >>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c >>>> index efdc0dbbcfc0..2cb7ba8f005a 100644 >>>> --- a/hw/ppc/spapr_caps.c >>>> +++ b/hw/ppc/spapr_caps.c >>> ... >>>> @@ -248,6 +249,7 @@ SpaprCapPossible cap_cfpc_possible = { >>>> static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, >>>> Error **errp) >>>> { >>>> + ERRP_AUTO_PROPAGATE(); >>>> Error *local_err = NULL; >>> >>> I think you should rename it, something like "local_warn" to not be >>> confused with the _auto_errp_prop.local_err... >>> >>> or don't use ERRP_AUTO_PROPAGE(), use the local_err instead and move the >>> warning inside the braces of the if. >>> >>> Same comment for cap_safe_bounds_check_apply() and >>> cap_safe_indirect_branch_apply() >>> >> >> Hmm... local_err isn't useful actually. It looks like we just want >> to call warn_report() directly instead of error_setg(&local_err) >> and warn_report_err(local_err). I'll post a v3. > > something like this I think: > > --- a/hw/ppc/spapr_caps.c > +++ b/hw/ppc/spapr_caps.c > @@ -250,24 +250,23 @@ static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, > Error **errp) > { > ERRP_AUTO_PROPAGATE(); > - Error *local_err = NULL; > uint8_t kvm_val = kvmppc_get_cap_safe_cache(); > > if (tcg_enabled() && val) { > /* TCG only supports broken, allow other values and print a warning */ > - error_setg(&local_err, > + error_setg(errp, > "TCG doesn't support requested feature, cap-cfpc=%s", > cap_cfpc_possible.vals[val]); > + if (*errp) { > + warn_report_err(*errp); > + *errp = NULL; > + } what a stupid code :) at least, if condition is always true. this all should be substitute by just warn_report("TCG doesn't support requested feature, cap-cfpc=%s", cap_cfpc_possible.vals[val]); > } else if (kvm_enabled() && (val > kvm_val)) { > error_setg(errp, > "Requested safe cache capability level not supported by KVM"); > error_append_hint(errp, "Try appending -machine cap-cfpc=%s\n", > cap_cfpc_possible.vals[kvm_val]); > } > - > - if (local_err != NULL) { > - warn_report_err(local_err); > - } > } > > > Or, we need to implement warn_report_errp() function, as I proposed in earlier version of auto-propagation series. > > ===== > > side idea: what if we make Error to be some kind of enum of pointer-to-pointer and pointer-to-function? > > Than, instead of passing pointers to error_abort and error_fatal as special casing, we'll pass pointers to functions, > which do appropriate handling of error. And we'll be able to pass warn_report function. Not about this patch set, > but seems interesting, isn't it? >
On Thu, 11 Jun 2020 13:42:48 +0300 Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > 11.06.2020 13:21, Vladimir Sementsov-Ogievskiy wrote: > > 11.06.2020 13:13, Greg Kurz wrote: > >> On Thu, 11 Jun 2020 11:50:57 +0200 > >> Laurent Vivier <lvivier@redhat.com> wrote: > >> > >>> On 11/06/2020 11:10, Greg Kurz wrote: > >>>> We have a dedicated error API for hints. Use it instead of embedding > >>>> the hint in the error message, as recommanded in the "qapi/error.h" > >>>> header file. > >>>> > >>>> Since spapr_caps_apply() passes &error_fatal, all functions must > >>>> also call the ERRP_AUTO_PROPAGATE() macro for error_append_hint() > >>>> to be functional. > >>>> > >>>> While here, add some missing braces around one line statements that > >>>> are part of the patch context. Also have cap_fwnmi_apply(), which > >>>> already uses error_append_hint() to call ERRP_AUTO_PROPAGATE() as > >>>> well. > >>>> > >>>> Signed-off-by: Greg Kurz <groug@kaod.org> > >>>> --- > >>>> hw/ppc/spapr_caps.c | 95 +++++++++++++++++++++++++++++---------------------- > >>>> 1 file changed, 54 insertions(+), 41 deletions(-) > >>>> > >>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c > >>>> index efdc0dbbcfc0..2cb7ba8f005a 100644 > >>>> --- a/hw/ppc/spapr_caps.c > >>>> +++ b/hw/ppc/spapr_caps.c > >>> ... > >>>> @@ -248,6 +249,7 @@ SpaprCapPossible cap_cfpc_possible = { > >>>> static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, > >>>> Error **errp) > >>>> { > >>>> + ERRP_AUTO_PROPAGATE(); > >>>> Error *local_err = NULL; > >>> > >>> I think you should rename it, something like "local_warn" to not be > >>> confused with the _auto_errp_prop.local_err... > >>> > >>> or don't use ERRP_AUTO_PROPAGE(), use the local_err instead and move the > >>> warning inside the braces of the if. > >>> > >>> Same comment for cap_safe_bounds_check_apply() and > >>> cap_safe_indirect_branch_apply() > >>> > >> > >> Hmm... local_err isn't useful actually. It looks like we just want > >> to call warn_report() directly instead of error_setg(&local_err) > >> and warn_report_err(local_err). I'll post a v3. > > > > something like this I think: > > > > --- a/hw/ppc/spapr_caps.c > > +++ b/hw/ppc/spapr_caps.c > > @@ -250,24 +250,23 @@ static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, > > Error **errp) > > { > > ERRP_AUTO_PROPAGATE(); > > - Error *local_err = NULL; > > uint8_t kvm_val = kvmppc_get_cap_safe_cache(); > > > > if (tcg_enabled() && val) { > > /* TCG only supports broken, allow other values and print a warning */ > > - error_setg(&local_err, > > + error_setg(errp, > > "TCG doesn't support requested feature, cap-cfpc=%s", > > cap_cfpc_possible.vals[val]); > > + if (*errp) { > > + warn_report_err(*errp); > > + *errp = NULL; > > + } > > what a stupid code :) at least, if condition is always true. > > this all should be substitute by just > > warn_report("TCG doesn't support requested feature, cap-cfpc=%s", cap_cfpc_possible.vals[val]); > Exactly ! :) > > > } else if (kvm_enabled() && (val > kvm_val)) { > > error_setg(errp, > > "Requested safe cache capability level not supported by KVM"); > > error_append_hint(errp, "Try appending -machine cap-cfpc=%s\n", > > cap_cfpc_possible.vals[kvm_val]); > > } > > - > > - if (local_err != NULL) { > > - warn_report_err(local_err); > > - } > > } > > > > > > Or, we need to implement warn_report_errp() function, as I proposed in earlier version of auto-propagation series. > > > > ===== > > > > side idea: what if we make Error to be some kind of enum of pointer-to-pointer and pointer-to-function? > > > > Than, instead of passing pointers to error_abort and error_fatal as special casing, we'll pass pointers to functions, > > which do appropriate handling of error. And we'll be able to pass warn_report function. Not about this patch set, > > but seems interesting, isn't it? > > > >
11.06.2020 13:39, Greg Kurz wrote: > On Thu, 11 Jun 2020 13:21:51 +0300 > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > >> 11.06.2020 13:13, Greg Kurz wrote: >>> On Thu, 11 Jun 2020 11:50:57 +0200 >>> Laurent Vivier <lvivier@redhat.com> wrote: >>> >>>> On 11/06/2020 11:10, Greg Kurz wrote: >>>>> We have a dedicated error API for hints. Use it instead of embedding >>>>> the hint in the error message, as recommanded in the "qapi/error.h" >>>>> header file. >>>>> >>>>> Since spapr_caps_apply() passes &error_fatal, all functions must >>>>> also call the ERRP_AUTO_PROPAGATE() macro for error_append_hint() >>>>> to be functional. >>>>> >>>>> While here, add some missing braces around one line statements that >>>>> are part of the patch context. Also have cap_fwnmi_apply(), which >>>>> already uses error_append_hint() to call ERRP_AUTO_PROPAGATE() as >>>>> well. >>>>> >>>>> Signed-off-by: Greg Kurz <groug@kaod.org> >>>>> --- >>>>> hw/ppc/spapr_caps.c | 95 +++++++++++++++++++++++++++++---------------------- >>>>> 1 file changed, 54 insertions(+), 41 deletions(-) >>>>> >>>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c >>>>> index efdc0dbbcfc0..2cb7ba8f005a 100644 >>>>> --- a/hw/ppc/spapr_caps.c >>>>> +++ b/hw/ppc/spapr_caps.c >>>> ... >>>>> @@ -248,6 +249,7 @@ SpaprCapPossible cap_cfpc_possible = { >>>>> static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, >>>>> Error **errp) >>>>> { >>>>> + ERRP_AUTO_PROPAGATE(); >>>>> Error *local_err = NULL; >>>> >>>> I think you should rename it, something like "local_warn" to not be >>>> confused with the _auto_errp_prop.local_err... >>>> >>>> or don't use ERRP_AUTO_PROPAGE(), use the local_err instead and move the >>>> warning inside the braces of the if. >>>> >>>> Same comment for cap_safe_bounds_check_apply() and >>>> cap_safe_indirect_branch_apply() >>>> >>> >>> Hmm... local_err isn't useful actually. It looks like we just want >>> to call warn_report() directly instead of error_setg(&local_err) >>> and warn_report_err(local_err). I'll post a v3. >> >> something like this I think: >> > > Not even that... we have one path (KVM) that directly > uses errp and the other path (TCG) that does: > > Error *local_err = NULL; > > error_setg(&local_err, ...); > > if (local_err) { > warn_report_err(local_err); > } > > It really looks like we just want to call warn_report(). yes, I also came this) > >> --- a/hw/ppc/spapr_caps.c >> +++ b/hw/ppc/spapr_caps.c >> @@ -250,24 +250,23 @@ static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, >> Error **errp) >> { >> ERRP_AUTO_PROPAGATE(); >> - Error *local_err = NULL; >> uint8_t kvm_val = kvmppc_get_cap_safe_cache(); >> >> if (tcg_enabled() && val) { >> /* TCG only supports broken, allow other values and print a warning */ >> - error_setg(&local_err, >> + error_setg(errp, >> "TCG doesn't support requested feature, cap-cfpc=%s", >> cap_cfpc_possible.vals[val]); >> + if (*errp) { >> + warn_report_err(*errp); >> + *errp = NULL; >> + } >> } else if (kvm_enabled() && (val > kvm_val)) { >> error_setg(errp, >> "Requested safe cache capability level not supported by KVM"); >> error_append_hint(errp, "Try appending -machine cap-cfpc=%s\n", >> cap_cfpc_possible.vals[kvm_val]); >> } >> - >> - if (local_err != NULL) { >> - warn_report_err(local_err); >> - } >> } >> >> >> Or, we need to implement warn_report_errp() function, as I proposed in earlier version of auto-propagation series. >> >> ===== >> >> side idea: what if we make Error to be some kind of enum of pointer-to-pointer and pointer-to-function? >> >> Than, instead of passing pointers to error_abort and error_fatal as special casing, we'll pass pointers to functions, >> which do appropriate handling of error. And we'll be able to pass warn_report function. Not about this patch set, >> but seems interesting, isn't it? >> >
11.06.2020 13:44, Vladimir Sementsov-Ogievskiy wrote: > 11.06.2020 13:39, Greg Kurz wrote: >> On Thu, 11 Jun 2020 13:21:51 +0300 >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: >> >>> 11.06.2020 13:13, Greg Kurz wrote: >>>> On Thu, 11 Jun 2020 11:50:57 +0200 >>>> Laurent Vivier <lvivier@redhat.com> wrote: >>>> >>>>> On 11/06/2020 11:10, Greg Kurz wrote: >>>>>> We have a dedicated error API for hints. Use it instead of embedding >>>>>> the hint in the error message, as recommanded in the "qapi/error.h" >>>>>> header file. >>>>>> >>>>>> Since spapr_caps_apply() passes &error_fatal, all functions must >>>>>> also call the ERRP_AUTO_PROPAGATE() macro for error_append_hint() >>>>>> to be functional. >>>>>> >>>>>> While here, add some missing braces around one line statements that >>>>>> are part of the patch context. Also have cap_fwnmi_apply(), which >>>>>> already uses error_append_hint() to call ERRP_AUTO_PROPAGATE() as >>>>>> well. >>>>>> >>>>>> Signed-off-by: Greg Kurz <groug@kaod.org> >>>>>> --- >>>>>> hw/ppc/spapr_caps.c | 95 +++++++++++++++++++++++++++++---------------------- >>>>>> 1 file changed, 54 insertions(+), 41 deletions(-) >>>>>> >>>>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c >>>>>> index efdc0dbbcfc0..2cb7ba8f005a 100644 >>>>>> --- a/hw/ppc/spapr_caps.c >>>>>> +++ b/hw/ppc/spapr_caps.c >>>>> ... >>>>>> @@ -248,6 +249,7 @@ SpaprCapPossible cap_cfpc_possible = { >>>>>> static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, >>>>>> Error **errp) >>>>>> { >>>>>> + ERRP_AUTO_PROPAGATE(); >>>>>> Error *local_err = NULL; >>>>> >>>>> I think you should rename it, something like "local_warn" to not be >>>>> confused with the _auto_errp_prop.local_err... >>>>> >>>>> or don't use ERRP_AUTO_PROPAGE(), use the local_err instead and move the >>>>> warning inside the braces of the if. >>>>> >>>>> Same comment for cap_safe_bounds_check_apply() and >>>>> cap_safe_indirect_branch_apply() >>>>> >>>> >>>> Hmm... local_err isn't useful actually. It looks like we just want >>>> to call warn_report() directly instead of error_setg(&local_err) >>>> and warn_report_err(local_err). I'll post a v3. >>> >>> something like this I think: >>> >> >> Not even that... we have one path (KVM) that directly >> uses errp and the other path (TCG) that does: >> >> Error *local_err = NULL; >> >> error_setg(&local_err, ...); >> >> if (local_err) { >> warn_report_err(local_err); >> } >> >> It really looks like we just want to call warn_report(). > > yes, I also came this) > >> >>> --- a/hw/ppc/spapr_caps.c >>> +++ b/hw/ppc/spapr_caps.c >>> @@ -250,24 +250,23 @@ static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, >>> Error **errp) >>> { >>> ERRP_AUTO_PROPAGATE(); >>> - Error *local_err = NULL; >>> uint8_t kvm_val = kvmppc_get_cap_safe_cache(); >>> if (tcg_enabled() && val) { >>> /* TCG only supports broken, allow other values and print a warning */ >>> - error_setg(&local_err, >>> + error_setg(errp, >>> "TCG doesn't support requested feature, cap-cfpc=%s", >>> cap_cfpc_possible.vals[val]); >>> + if (*errp) { >>> + warn_report_err(*errp); >>> + *errp = NULL; >>> + } >>> } else if (kvm_enabled() && (val > kvm_val)) { >>> error_setg(errp, >>> "Requested safe cache capability level not supported by KVM"); >>> error_append_hint(errp, "Try appending -machine cap-cfpc=%s\n", >>> cap_cfpc_possible.vals[kvm_val]); >>> } >>> - >>> - if (local_err != NULL) { >>> - warn_report_err(local_err); >>> - } >>> } >>> >>> >>> Or, we need to implement warn_report_errp() function, as I proposed in earlier version of auto-propagation series. >>> >>> ===== >>> >>> side idea: what if we make Error to be some kind of enum of pointer-to-pointer and pointer-to-function? >>> >>> Than, instead of passing pointers to error_abort and error_fatal as special casing, we'll pass pointers to functions, >>> which do appropriate handling of error. And we'll be able to pass warn_report function. Not about this patch set, >>> but seems interesting, isn't it? >>> >> > Just for record: probably, we'll want at lest something like this (not needed in this series): diff --git a/include/qapi/error.h b/include/qapi/error.h index 30140d9bfe..25b5da0733 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -475,4 +475,10 @@ extern Error *error_abort; */ extern Error *error_fatal; +/* + * Special error destination to call warn_report_err. + * See error_setg() and error_propagate() for details. + */ +extern Error *error_warn_report; + #endif diff --git a/util/error.c b/util/error.c index b6c89d1412..928eb4cc7a 100644 --- a/util/error.c +++ b/util/error.c @@ -27,8 +27,9 @@ struct Error Error *error_abort; Error *error_fatal; +Error *error_warn_report; -static void error_handle_fatal(Error **errp, Error *err) +static bool error_handle_special(Error **errp, Error *err) { if (errp == &error_abort) { fprintf(stderr, "Unexpected error in %s() at %s:%d:\n", @@ -43,6 +44,12 @@ static void error_handle_fatal(Error **errp, Error *err) error_report_err(err); exit(1); } + if (errp == &error_warn_report) { + warn_report_err(err); + return true; + } + + return false; } static void error_setv(Error **errp, @@ -70,8 +77,9 @@ static void error_setv(Error **errp, err->line = line; err->func = func; - error_handle_fatal(errp, err); - *errp = err; + if (!error_handle_special(errp, err)) { + *errp = err; + } errno = saved_errno; } @@ -283,7 +291,9 @@ void error_propagate(Error **dst_errp, Error *local_err) if (!local_err) { return; } - error_handle_fatal(dst_errp, local_err); + if (error_handle_special(dst_errp, local_err)) { + return; + } if (dst_errp && !*dst_errp) { *dst_errp = local_err; } else {
On 11/06/2020 12:44, Greg Kurz wrote: > On Thu, 11 Jun 2020 13:42:48 +0300 > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > >> 11.06.2020 13:21, Vladimir Sementsov-Ogievskiy wrote: >>> 11.06.2020 13:13, Greg Kurz wrote: >>>> On Thu, 11 Jun 2020 11:50:57 +0200 >>>> Laurent Vivier <lvivier@redhat.com> wrote: >>>> >>>>> On 11/06/2020 11:10, Greg Kurz wrote: >>>>>> We have a dedicated error API for hints. Use it instead of embedding >>>>>> the hint in the error message, as recommanded in the "qapi/error.h" >>>>>> header file. >>>>>> >>>>>> Since spapr_caps_apply() passes &error_fatal, all functions must >>>>>> also call the ERRP_AUTO_PROPAGATE() macro for error_append_hint() >>>>>> to be functional. >>>>>> >>>>>> While here, add some missing braces around one line statements that >>>>>> are part of the patch context. Also have cap_fwnmi_apply(), which >>>>>> already uses error_append_hint() to call ERRP_AUTO_PROPAGATE() as >>>>>> well. >>>>>> >>>>>> Signed-off-by: Greg Kurz <groug@kaod.org> >>>>>> --- >>>>>> hw/ppc/spapr_caps.c | 95 +++++++++++++++++++++++++++++---------------------- >>>>>> 1 file changed, 54 insertions(+), 41 deletions(-) >>>>>> >>>>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c >>>>>> index efdc0dbbcfc0..2cb7ba8f005a 100644 >>>>>> --- a/hw/ppc/spapr_caps.c >>>>>> +++ b/hw/ppc/spapr_caps.c >>>>> ... >>>>>> @@ -248,6 +249,7 @@ SpaprCapPossible cap_cfpc_possible = { >>>>>> static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, >>>>>> Error **errp) >>>>>> { >>>>>> + ERRP_AUTO_PROPAGATE(); >>>>>> Error *local_err = NULL; >>>>> >>>>> I think you should rename it, something like "local_warn" to not be >>>>> confused with the _auto_errp_prop.local_err... >>>>> >>>>> or don't use ERRP_AUTO_PROPAGE(), use the local_err instead and move the >>>>> warning inside the braces of the if. >>>>> >>>>> Same comment for cap_safe_bounds_check_apply() and >>>>> cap_safe_indirect_branch_apply() >>>>> >>>> >>>> Hmm... local_err isn't useful actually. It looks like we just want >>>> to call warn_report() directly instead of error_setg(&local_err) >>>> and warn_report_err(local_err). I'll post a v3. >>> >>> something like this I think: >>> >>> --- a/hw/ppc/spapr_caps.c >>> +++ b/hw/ppc/spapr_caps.c >>> @@ -250,24 +250,23 @@ static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, >>> Error **errp) >>> { >>> ERRP_AUTO_PROPAGATE(); >>> - Error *local_err = NULL; >>> uint8_t kvm_val = kvmppc_get_cap_safe_cache(); >>> >>> if (tcg_enabled() && val) { >>> /* TCG only supports broken, allow other values and print a warning */ >>> - error_setg(&local_err, >>> + error_setg(errp, >>> "TCG doesn't support requested feature, cap-cfpc=%s", >>> cap_cfpc_possible.vals[val]); >>> + if (*errp) { >>> + warn_report_err(*errp); >>> + *errp = NULL; >>> + } >> >> what a stupid code :) at least, if condition is always true. >> >> this all should be substitute by just >> >> warn_report("TCG doesn't support requested feature, cap-cfpc=%s", cap_cfpc_possible.vals[val]); >> > > Exactly ! :) > Yes, it seems appropriate. Thanks, Laurent
diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c index efdc0dbbcfc0..2cb7ba8f005a 100644 --- a/hw/ppc/spapr_caps.c +++ b/hw/ppc/spapr_caps.c @@ -189,24 +189,24 @@ static void spapr_cap_set_pagesize(Object *obj, Visitor *v, const char *name, static void cap_htm_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) { + ERRP_AUTO_PROPAGATE(); if (!val) { /* TODO: We don't support disabling htm yet */ return; } if (tcg_enabled()) { - error_setg(errp, - "No Transactional Memory support in TCG," - " try appending -machine cap-htm=off"); + error_setg(errp, "No Transactional Memory support in TCG"); + error_append_hint(errp, "Try appending -machine cap-htm=off\n"); } else if (kvm_enabled() && !kvmppc_has_cap_htm()) { error_setg(errp, -"KVM implementation does not support Transactional Memory," - " try appending -machine cap-htm=off" - ); + "KVM implementation does not support Transactional Memory"); + error_append_hint(errp, "Try appending -machine cap-htm=off\n"); } } static void cap_vsx_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) { + ERRP_AUTO_PROPAGATE(); PowerPCCPU *cpu = POWERPC_CPU(first_cpu); CPUPPCState *env = &cpu->env; @@ -218,13 +218,14 @@ static void cap_vsx_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) * rid of anything that doesn't do VMX */ g_assert(env->insns_flags & PPC_ALTIVEC); if (!(env->insns_flags2 & PPC2_VSX)) { - error_setg(errp, "VSX support not available," - " try appending -machine cap-vsx=off"); + error_setg(errp, "VSX support not available"); + error_append_hint(errp, "Try appending -machine cap-vsx=off\n"); } } static void cap_dfp_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) { + ERRP_AUTO_PROPAGATE(); PowerPCCPU *cpu = POWERPC_CPU(first_cpu); CPUPPCState *env = &cpu->env; @@ -233,8 +234,8 @@ static void cap_dfp_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) return; } if (!(env->insns_flags2 & PPC2_DFP)) { - error_setg(errp, "DFP support not available," - " try appending -machine cap-dfp=off"); + error_setg(errp, "DFP support not available"); + error_append_hint(errp, "Try appending -machine cap-dfp=off\n"); } } @@ -248,6 +249,7 @@ SpaprCapPossible cap_cfpc_possible = { static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) { + ERRP_AUTO_PROPAGATE(); Error *local_err = NULL; uint8_t kvm_val = kvmppc_get_cap_safe_cache(); @@ -258,13 +260,14 @@ static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, cap_cfpc_possible.vals[val]); } else if (kvm_enabled() && (val > kvm_val)) { error_setg(errp, - "Requested safe cache capability level not supported by kvm," - " try appending -machine cap-cfpc=%s", - cap_cfpc_possible.vals[kvm_val]); + "Requested safe cache capability level not supported by KVM"); + error_append_hint(errp, "Try appending -machine cap-cfpc=%s\n", + cap_cfpc_possible.vals[kvm_val]); } - if (local_err != NULL) + if (local_err != NULL) { warn_report_err(local_err); + } } SpaprCapPossible cap_sbbc_possible = { @@ -277,6 +280,7 @@ SpaprCapPossible cap_sbbc_possible = { static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) { + ERRP_AUTO_PROPAGATE(); Error *local_err = NULL; uint8_t kvm_val = kvmppc_get_cap_safe_bounds_check(); @@ -287,13 +291,14 @@ static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val, cap_sbbc_possible.vals[val]); } else if (kvm_enabled() && (val > kvm_val)) { error_setg(errp, -"Requested safe bounds check capability level not supported by kvm," - " try appending -machine cap-sbbc=%s", - cap_sbbc_possible.vals[kvm_val]); +"Requested safe bounds check capability level not supported by KVM"); + error_append_hint(errp, "Try appending -machine cap-sbbc=%s\n", + cap_sbbc_possible.vals[kvm_val]); } - if (local_err != NULL) + if (local_err != NULL) { warn_report_err(local_err); + } } SpaprCapPossible cap_ibs_possible = { @@ -309,6 +314,7 @@ SpaprCapPossible cap_ibs_possible = { static void cap_safe_indirect_branch_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) { + ERRP_AUTO_PROPAGATE(); Error *local_err = NULL; uint8_t kvm_val = kvmppc_get_cap_safe_indirect_branch(); @@ -319,9 +325,9 @@ static void cap_safe_indirect_branch_apply(SpaprMachineState *spapr, cap_ibs_possible.vals[val]); } else if (kvm_enabled() && (val > kvm_val)) { error_setg(errp, -"Requested safe indirect branch capability level not supported by kvm," - " try appending -machine cap-ibs=%s", - cap_ibs_possible.vals[kvm_val]); +"Requested safe indirect branch capability level not supported by KVM"); + error_append_hint(errp, "Try appending -machine cap-ibs=%s\n", + cap_ibs_possible.vals[kvm_val]); } if (local_err != NULL) { @@ -402,23 +408,25 @@ static void cap_hpt_maxpagesize_cpu_apply(SpaprMachineState *spapr, static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) { + ERRP_AUTO_PROPAGATE(); if (!val) { /* capability disabled by default */ return; } if (tcg_enabled()) { - error_setg(errp, - "No Nested KVM-HV support in tcg," - " try appending -machine cap-nested-hv=off"); + error_setg(errp, "No Nested KVM-HV support in TCG"); + error_append_hint(errp, "Try appending -machine cap-nested-hv=off\n"); } else if (kvm_enabled()) { if (!kvmppc_has_cap_nested_kvm_hv()) { error_setg(errp, -"KVM implementation does not support Nested KVM-HV," - " try appending -machine cap-nested-hv=off"); + "KVM implementation does not support Nested KVM-HV"); + error_append_hint(errp, + "Try appending -machine cap-nested-hv=off\n"); } else if (kvmppc_set_cap_nested_kvm_hv(val) < 0) { - error_setg(errp, -"Error enabling cap-nested-hv with KVM, try cap-nested-hv=off"); + error_setg(errp, "Error enabling cap-nested-hv with KVM"); + error_append_hint(errp, + "Try appending -machine cap-nested-hv=off\n"); } } } @@ -426,6 +434,7 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr, static void cap_large_decr_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) { + ERRP_AUTO_PROPAGATE(); PowerPCCPU *cpu = POWERPC_CPU(first_cpu); PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); @@ -436,22 +445,23 @@ static void cap_large_decr_apply(SpaprMachineState *spapr, if (tcg_enabled()) { if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_00, 0, spapr->max_compat_pvr)) { - error_setg(errp, - "Large decrementer only supported on POWER9, try -cpu POWER9"); + error_setg(errp, "Large decrementer only supported on POWER9"); + error_append_hint(errp, "Try -cpu POWER9\n"); return; } } else if (kvm_enabled()) { int kvm_nr_bits = kvmppc_get_cap_large_decr(); if (!kvm_nr_bits) { - error_setg(errp, - "No large decrementer support," - " try appending -machine cap-large-decr=off"); + error_setg(errp, "No large decrementer support"); + error_append_hint(errp, + "Try appending -machine cap-large-decr=off\n"); } else if (pcc->lrg_decr_bits != kvm_nr_bits) { error_setg(errp, -"KVM large decrementer size (%d) differs to model (%d)," - " try appending -machine cap-large-decr=off", - kvm_nr_bits, pcc->lrg_decr_bits); + "KVM large decrementer size (%d) differs to model (%d)", + kvm_nr_bits, pcc->lrg_decr_bits); + error_append_hint(errp, + "Try appending -machine cap-large-decr=off\n"); } } } @@ -460,14 +470,15 @@ static void cap_large_decr_cpu_apply(SpaprMachineState *spapr, PowerPCCPU *cpu, uint8_t val, Error **errp) { + ERRP_AUTO_PROPAGATE(); CPUPPCState *env = &cpu->env; target_ulong lpcr = env->spr[SPR_LPCR]; if (kvm_enabled()) { if (kvmppc_enable_cap_large_decr(cpu, val)) { - error_setg(errp, - "No large decrementer support," - " try appending -machine cap-large-decr=off"); + error_setg(errp, "No large decrementer support"); + error_append_hint(errp, + "Try appending -machine cap-large-decr=off\n"); } } @@ -482,6 +493,7 @@ static void cap_large_decr_cpu_apply(SpaprMachineState *spapr, static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) { + ERRP_AUTO_PROPAGATE(); uint8_t kvm_val = kvmppc_get_cap_count_cache_flush_assist(); if (tcg_enabled() && val) { @@ -504,14 +516,15 @@ static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val, return; } error_setg(errp, -"Requested count cache flush assist capability level not supported by kvm," - " try appending -machine cap-ccf-assist=off"); + "Requested count cache flush assist capability level not supported by KVM"); + error_append_hint(errp, "Try appending -machine cap-ccf-assist=off\n"); } } static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) { + ERRP_AUTO_PROPAGATE(); if (!val) { return; /* Disabled by default */ }
We have a dedicated error API for hints. Use it instead of embedding the hint in the error message, as recommanded in the "qapi/error.h" header file. Since spapr_caps_apply() passes &error_fatal, all functions must also call the ERRP_AUTO_PROPAGATE() macro for error_append_hint() to be functional. While here, add some missing braces around one line statements that are part of the patch context. Also have cap_fwnmi_apply(), which already uses error_append_hint() to call ERRP_AUTO_PROPAGATE() as well. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/ppc/spapr_caps.c | 95 +++++++++++++++++++++++++++++---------------------- 1 file changed, 54 insertions(+), 41 deletions(-)