On Thu, 11 Jun 2020 11:50:57 +0200
Laurent Vivier <lviv...@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 <gr...@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
>
