Re: [PATCH v2 2/3] spapr: Use error_append_hint() in spapr_caps.c

[Vladimir Sementsov-Ogievskiy]

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?


--
Best regards,
Vladimir