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

[Laurent Vivier <lvivier@redhat.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