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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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?

-- 
Best regards,
Vladimir