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

[Greg Kurz]

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?
>