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

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

11.06.2020 13:44, Vladimir Sementsov-Ogievskiy wrote:
> 11.06.2020 13:39, Greg Kurz wrote:
>> 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().
> 
> yes, I also came this)
> 
>>
>>> --- 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?
>>>
>>
> 

Just for record: probably, we'll want at lest something like this (not needed in this series):

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 30140d9bfe..25b5da0733 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -475,4 +475,10 @@ extern Error *error_abort;
   */
  extern Error *error_fatal;
  
+/*
+ * Special error destination to call warn_report_err.
+ * See error_setg() and error_propagate() for details.
+ */
+extern Error *error_warn_report;
+
  #endif
diff --git a/util/error.c b/util/error.c
index b6c89d1412..928eb4cc7a 100644
--- a/util/error.c
+++ b/util/error.c
@@ -27,8 +27,9 @@ struct Error
  
  Error *error_abort;
  Error *error_fatal;
+Error *error_warn_report;
  
-static void error_handle_fatal(Error **errp, Error *err)
+static bool error_handle_special(Error **errp, Error *err)
  {
      if (errp == &error_abort) {
          fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
@@ -43,6 +44,12 @@ static void error_handle_fatal(Error **errp, Error *err)
          error_report_err(err);
          exit(1);
      }
+    if (errp == &error_warn_report) {
+        warn_report_err(err);
+        return true;
+    }
+
+    return false;
  }
  
  static void error_setv(Error **errp,
@@ -70,8 +77,9 @@ static void error_setv(Error **errp,
      err->line = line;
      err->func = func;
  
-    error_handle_fatal(errp, err);
-    *errp = err;
+    if (!error_handle_special(errp, err)) {
+        *errp = err;
+    }
  
      errno = saved_errno;
  }
@@ -283,7 +291,9 @@ void error_propagate(Error **dst_errp, Error *local_err)
      if (!local_err) {
          return;
      }
-    error_handle_fatal(dst_errp, local_err);
+    if (error_handle_special(dst_errp, local_err)) {
+        return;
+    }
      if (dst_errp && !*dst_errp) {
          *dst_errp = local_err;
      } else {



-- 
Best regards,
Vladimir