[PATCH v2 0/3] spapr: Improve error reporting in spapr_caps.c

[PATCH v2 0/3] spapr: Improve error reporting in spapr_caps.c

[Greg Kurz]

Spapr capabilities are checked at machine init. If a capability cannot
be used, an error message is printed and QEMU exits. In most places,
the error message also contains an hint for the user. But we should
use error_append_hint() for that, as explained in the "qapi/error.h"
header.

This is already the case for cap_fwnmi_apply() and we now want to add 
a similar check for nested KVM-HV. Unfortunately, spapr_caps_apply()
passes &error_fatal to all apply hooks and error_append_hint() is
never called. 

So this reuses previous work from Vladimir Sementsov-Ogievskiy to
address that.

v2: - Fix indentation and add some missing \n in patch 2
    - Add ERRP_AUTO_PROPAGATE() to cap_nested_kvm_hv_apply() in
      patch 2 instead of patch 3
---

Greg Kurz (2):
      spapr: Use error_append_hint() in spapr_caps.c
      spapr: Forbid nested KVM-HV in pre-power9 compat mode

Vladimir Sementsov-Ogievskiy (1):
      error: auto propagated local_err


 hw/ppc/spapr_caps.c  |  105 ++++++++++++++++----------
 include/qapi/error.h |  205 ++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 237 insertions(+), 73 deletions(-)

--
Greg

[PATCH v2 1/3] error: auto propagated local_err

[Greg Kurz]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Introduce a new ERRP_AUTO_PROPAGATE macro, to be used at start of
functions with an errp OUT parameter.

It has three goals:

1. Fix issue with error_fatal and error_prepend/error_append_hint: user
can't see this additional information, because exit() happens in
error_setg earlier than information is added. [Reported by Greg Kurz]

2. Fix issue with error_abort and error_propagate: when we wrap
error_abort by local_err+error_propagate, the resulting coredump will
refer to error_propagate and not to the place where error happened.
(the macro itself doesn't fix the issue, but it allows us to [3.] drop
the local_err+error_propagate pattern, which will definitely fix the
issue) [Reported by Kevin Wolf]

3. Drop local_err+error_propagate pattern, which is used to workaround
void functions with errp parameter, when caller wants to know resulting
status. (Note: actually these functions could be merely updated to
return int error code).

To achieve these goals, later patches will add invocations
of this macro at the start of functions with either use
error_prepend/error_append_hint (solving 1) or which use
local_err+error_propagate to check errors, switching those
functions to use *errp instead (solving 2 and 3).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Paul Durrant <paul@xen.org>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 include/qapi/error.h |  205 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 173 insertions(+), 32 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index ad5b6e896ded..30140d9bfea9 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -15,6 +15,8 @@
 /*
  * Error reporting system loosely patterned after Glib's GError.
  *
+ * = Deal with Error object =
+ *
  * Create an error:
  *     error_setg(&err, "situation normal, all fouled up");
  *
@@ -47,28 +49,91 @@
  * reporting it (primarily useful in testsuites):
  *     error_free_or_abort(&err);
  *
- * Pass an existing error to the caller:
- *     error_propagate(errp, err);
- * where Error **errp is a parameter, by convention the last one.
+ * = Deal with Error ** function parameter =
  *
- * Pass an existing error to the caller with the message modified:
- *     error_propagate_prepend(errp, err);
+ * A function may use the error system to return errors. In this case, the
+ * function defines an Error **errp parameter, by convention the last one (with
+ * exceptions for functions using ... or va_list).
  *
- * Avoid
- *     error_propagate(errp, err);
- *     error_prepend(errp, "Could not frobnicate '%s': ", name);
- * because this fails to prepend when @errp is &error_fatal.
+ * The caller may then pass in the following errp values:
  *
- * Create a new error and pass it to the caller:
+ * 1. &error_abort
+ *    Any error will result in abort().
+ * 2. &error_fatal
+ *    Any error will result in exit() with a non-zero status.
+ * 3. NULL
+ *    No error reporting through errp parameter.
+ * 4. The address of a NULL-initialized Error *err
+ *    Any error will populate errp with an error object.
+ *
+ * The following rules then implement the correct semantics desired by the
+ * caller.
+ *
+ * Create a new error to pass to the caller:
  *     error_setg(errp, "situation normal, all fouled up");
  *
- * Call a function and receive an error from it:
+ * Calling another errp-based function:
+ *     f(..., errp);
+ *
+ * == Checking success of subcall ==
+ *
+ * If a function returns a value indicating an error in addition to setting
+ * errp (which is recommended), then you don't need any additional code, just
+ * do:
+ *
+ *     int ret = f(..., errp);
+ *     if (ret < 0) {
+ *         ... handle error ...
+ *         return ret;
+ *     }
+ *
+ * If a function returns nothing (not recommended for new code), the only way
+ * to check success is by consulting errp; doing this safely requires the use
+ * of the ERRP_AUTO_PROPAGATE macro, like this:
+ *
+ *     int our_func(..., Error **errp) {
+ *         ERRP_AUTO_PROPAGATE();
+ *         ...
+ *         subcall(..., errp);
+ *         if (*errp) {
+ *             ...
+ *             return -EINVAL;
+ *         }
+ *         ...
+ *     }
+ *
+ * ERRP_AUTO_PROPAGATE takes care of wrapping the original errp as needed, so
+ * that the rest of the function can directly use errp (including
+ * dereferencing), where any errors will then be propagated on to the original
+ * errp when leaving the function.
+ *
+ * In some cases, we need to check result of subcall, but do not want to
+ * propagate the Error object to our caller. In such cases we don't need
+ * ERRP_AUTO_PROPAGATE, but just a local Error object:
+ *
+ * Receive an error and not pass it:
  *     Error *err = NULL;
- *     foo(arg, &err);
+ *     subcall(arg, &err);
  *     if (err) {
  *         handle the error...
+ *         error_free(err);
  *     }
  *
+ * Note that older code that did not use ERRP_AUTO_PROPAGATE would instead need
+ * a local Error * variable and the use of error_propagate() to properly handle
+ * all possible caller values of errp. Now this is DEPRECATED* (see below).
+ *
+ * Note that any function that wants to modify an error object, such as by
+ * calling error_append_hint or error_prepend, must use ERRP_AUTO_PROPAGATE, in
+ * order for a caller's use of &error_fatal to see the additional information.
+ *
+ * In rare cases, we need to pass existing Error object to the caller by hand:
+ *     error_propagate(errp, err);
+ *
+ * Pass an existing error to the caller with the message modified:
+ *     error_propagate_prepend(errp, err);
+ *
+ *
  * Call a function ignoring errors:
  *     foo(arg, NULL);
  *
@@ -78,26 +143,6 @@
  * Call a function treating errors as fatal:
  *     foo(arg, &error_fatal);
  *
- * Receive an error and pass it on to the caller:
- *     Error *err = NULL;
- *     foo(arg, &err);
- *     if (err) {
- *         handle the error...
- *         error_propagate(errp, err);
- *     }
- * where Error **errp is a parameter, by convention the last one.
- *
- * Do *not* "optimize" this to
- *     foo(arg, errp);
- *     if (*errp) { // WRONG!
- *         handle the error...
- *     }
- * because errp may be NULL!
- *
- * But when all you do with the error is pass it on, please use
- *     foo(arg, errp);
- * for readability.
- *
  * Receive and accumulate multiple errors (first one wins):
  *     Error *err = NULL, *local_err = NULL;
  *     foo(arg, &err);
@@ -114,6 +159,61 @@
  *         handle the error...
  *     }
  * because this may pass a non-null err to bar().
+ *
+ * DEPRECATED*
+ *
+ * The following pattern of receiving, checking, and then forwarding an error
+ * to the caller by hand is now deprecated:
+ *
+ *     Error *err = NULL;
+ *     foo(arg, &err);
+ *     if (err) {
+ *         handle the error...
+ *         error_propagate(errp, err);
+ *     }
+ *
+ * Instead, use ERRP_AUTO_PROPAGATE macro.
+ *
+ * The old pattern is deprecated because of two things:
+ *
+ * 1. Issue with error_abort and error_propagate: when we wrap error_abort by
+ * local_err+error_propagate, the resulting coredump will refer to
+ * error_propagate and not to the place where error happened.
+ *
+ * 2. A lot of extra code of the same pattern
+ *
+ * How to update old code to use ERRP_AUTO_PROPAGATE?
+ *
+ * All you need is to add ERRP_AUTO_PROPAGATE() invocation at function start,
+ * than you may safely dereference errp to check errors and do not need any
+ * additional local Error variables or calls to error_propagate().
+ *
+ * Example:
+ *
+ * old code
+ *
+ *     void fn(..., Error **errp) {
+ *         Error *err = NULL;
+ *         foo(arg, &err);
+ *         if (err) {
+ *             handle the error...
+ *             error_propagate(errp, err);
+ *             return;
+ *         }
+ *         ...
+ *     }
+ *
+ * updated code
+ *
+ *     void fn(..., Error **errp) {
+ *         ERRP_AUTO_PROPAGATE();
+ *         foo(arg, errp);
+ *         if (*errp) {
+ *             handle the error...
+ *             return;
+ *         }
+ *         ...
+ *     }
  */
 
 #ifndef ERROR_H
@@ -322,6 +422,47 @@ void error_set_internal(Error **errp,
                         ErrorClass err_class, const char *fmt, ...)
     GCC_FMT_ATTR(6, 7);
 
+typedef struct ErrorPropagator {
+    Error *local_err;
+    Error **errp;
+} ErrorPropagator;
+
+static inline void error_propagator_cleanup(ErrorPropagator *prop)
+{
+    error_propagate(prop->errp, prop->local_err);
+}
+
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
+
+/*
+ * ERRP_AUTO_PROPAGATE
+ *
+ * This macro exists to assist with proper error handling in a function which
+ * uses an Error **errp parameter.  It must be used as the first line of a
+ * function which modifies an error (with error_prepend, error_append_hint, or
+ * similar) or which wants to dereference *errp.  It is still safe (but
+ * useless) to use in other functions.
+ *
+ * If errp is NULL or points to error_fatal, it is rewritten to point to a
+ * local Error object, which will be automatically propagated to the original
+ * errp on function exit (see error_propagator_cleanup).
+ *
+ * After invocation of this macro it is always safe to dereference errp
+ * (as it's not NULL anymore) and to add information by error_prepend or
+ * error_append_hint (as, if it was error_fatal, we swapped it with a
+ * local_error to be propagated on cleanup).
+ *
+ * Note: we don't wrap the error_abort case, as we want resulting coredump
+ * to point to the place where the error happened, not to error_propagate.
+ */
+#define ERRP_AUTO_PROPAGATE() \
+    g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp}; \
+    do { \
+        if (!errp || errp == &error_fatal) { \
+            errp = &_auto_errp_prop.local_err; \
+        } \
+    } while (0)
+
 /*
  * Special error destination to abort on error.
  * See error_setg() and error_propagate() for details.

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

[Greg Kurz]

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
@@ -189,24 +189,24 @@ static void spapr_cap_set_pagesize(Object *obj, Visitor *v, const char *name,
 
 static void cap_htm_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     if (!val) {
         /* TODO: We don't support disabling htm yet */
         return;
     }
     if (tcg_enabled()) {
-        error_setg(errp,
-                   "No Transactional Memory support in TCG,"
-                   " try appending -machine cap-htm=off");
+        error_setg(errp, "No Transactional Memory support in TCG");
+        error_append_hint(errp, "Try appending -machine cap-htm=off\n");
     } else if (kvm_enabled() && !kvmppc_has_cap_htm()) {
         error_setg(errp,
-"KVM implementation does not support Transactional Memory,"
-                   " try appending -machine cap-htm=off"
-            );
+                   "KVM implementation does not support Transactional Memory");
+        error_append_hint(errp, "Try appending -machine cap-htm=off\n");
     }
 }
 
 static void cap_vsx_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
     CPUPPCState *env = &cpu->env;
 
@@ -218,13 +218,14 @@ static void cap_vsx_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
      * rid of anything that doesn't do VMX */
     g_assert(env->insns_flags & PPC_ALTIVEC);
     if (!(env->insns_flags2 & PPC2_VSX)) {
-        error_setg(errp, "VSX support not available,"
-                   " try appending -machine cap-vsx=off");
+        error_setg(errp, "VSX support not available");
+        error_append_hint(errp, "Try appending -machine cap-vsx=off\n");
     }
 }
 
 static void cap_dfp_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
     CPUPPCState *env = &cpu->env;
 
@@ -233,8 +234,8 @@ static void cap_dfp_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
         return;
     }
     if (!(env->insns_flags2 & PPC2_DFP)) {
-        error_setg(errp, "DFP support not available,"
-                   " try appending -machine cap-dfp=off");
+        error_setg(errp, "DFP support not available");
+        error_append_hint(errp, "Try appending -machine cap-dfp=off\n");
     }
 }
 
@@ -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;
     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 = {
@@ -277,6 +280,7 @@ SpaprCapPossible cap_sbbc_possible = {
 static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val,
                                         Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     Error *local_err = NULL;
     uint8_t kvm_val =  kvmppc_get_cap_safe_bounds_check();
 
@@ -287,13 +291,14 @@ static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val,
                    cap_sbbc_possible.vals[val]);
     } else if (kvm_enabled() && (val > kvm_val)) {
         error_setg(errp,
-"Requested safe bounds check capability level not supported by kvm,"
-                   " try appending -machine cap-sbbc=%s",
-                   cap_sbbc_possible.vals[kvm_val]);
+"Requested safe bounds check capability level not supported by KVM");
+        error_append_hint(errp, "Try appending -machine cap-sbbc=%s\n",
+                          cap_sbbc_possible.vals[kvm_val]);
     }
 
-    if (local_err != NULL)
+    if (local_err != NULL) {
         warn_report_err(local_err);
+    }
 }
 
 SpaprCapPossible cap_ibs_possible = {
@@ -309,6 +314,7 @@ SpaprCapPossible cap_ibs_possible = {
 static void cap_safe_indirect_branch_apply(SpaprMachineState *spapr,
                                            uint8_t val, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     Error *local_err = NULL;
     uint8_t kvm_val = kvmppc_get_cap_safe_indirect_branch();
 
@@ -319,9 +325,9 @@ static void cap_safe_indirect_branch_apply(SpaprMachineState *spapr,
                    cap_ibs_possible.vals[val]);
     } else if (kvm_enabled() && (val > kvm_val)) {
         error_setg(errp,
-"Requested safe indirect branch capability level not supported by kvm,"
-                   " try appending -machine cap-ibs=%s",
-                   cap_ibs_possible.vals[kvm_val]);
+"Requested safe indirect branch capability level not supported by KVM");
+        error_append_hint(errp, "Try appending -machine cap-ibs=%s\n",
+                          cap_ibs_possible.vals[kvm_val]);
     }
 
     if (local_err != NULL) {
@@ -402,23 +408,25 @@ static void cap_hpt_maxpagesize_cpu_apply(SpaprMachineState *spapr,
 static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
                                     uint8_t val, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     if (!val) {
         /* capability disabled by default */
         return;
     }
 
     if (tcg_enabled()) {
-        error_setg(errp,
-                   "No Nested KVM-HV support in tcg,"
-                   " try appending -machine cap-nested-hv=off");
+        error_setg(errp, "No Nested KVM-HV support in TCG");
+        error_append_hint(errp, "Try appending -machine cap-nested-hv=off\n");
     } else if (kvm_enabled()) {
         if (!kvmppc_has_cap_nested_kvm_hv()) {
             error_setg(errp,
-"KVM implementation does not support Nested KVM-HV,"
-                       " try appending -machine cap-nested-hv=off");
+                       "KVM implementation does not support Nested KVM-HV");
+            error_append_hint(errp,
+                              "Try appending -machine cap-nested-hv=off\n");
         } else if (kvmppc_set_cap_nested_kvm_hv(val) < 0) {
-                error_setg(errp,
-"Error enabling cap-nested-hv with KVM, try cap-nested-hv=off");
+                error_setg(errp, "Error enabling cap-nested-hv with KVM");
+                error_append_hint(errp,
+                                  "Try appending -machine cap-nested-hv=off\n");
         }
     }
 }
@@ -426,6 +434,7 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
 static void cap_large_decr_apply(SpaprMachineState *spapr,
                                  uint8_t val, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
 
@@ -436,22 +445,23 @@ static void cap_large_decr_apply(SpaprMachineState *spapr,
     if (tcg_enabled()) {
         if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_00, 0,
                               spapr->max_compat_pvr)) {
-            error_setg(errp,
-                "Large decrementer only supported on POWER9, try -cpu POWER9");
+            error_setg(errp, "Large decrementer only supported on POWER9");
+            error_append_hint(errp, "Try -cpu POWER9\n");
             return;
         }
     } else if (kvm_enabled()) {
         int kvm_nr_bits = kvmppc_get_cap_large_decr();
 
         if (!kvm_nr_bits) {
-            error_setg(errp,
-                       "No large decrementer support,"
-                        " try appending -machine cap-large-decr=off");
+            error_setg(errp, "No large decrementer support");
+            error_append_hint(errp,
+                              "Try appending -machine cap-large-decr=off\n");
         } else if (pcc->lrg_decr_bits != kvm_nr_bits) {
             error_setg(errp,
-"KVM large decrementer size (%d) differs to model (%d),"
-                " try appending -machine cap-large-decr=off",
-                kvm_nr_bits, pcc->lrg_decr_bits);
+                       "KVM large decrementer size (%d) differs to model (%d)",
+                       kvm_nr_bits, pcc->lrg_decr_bits);
+            error_append_hint(errp,
+                              "Try appending -machine cap-large-decr=off\n");
         }
     }
 }
@@ -460,14 +470,15 @@ static void cap_large_decr_cpu_apply(SpaprMachineState *spapr,
                                      PowerPCCPU *cpu,
                                      uint8_t val, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     CPUPPCState *env = &cpu->env;
     target_ulong lpcr = env->spr[SPR_LPCR];
 
     if (kvm_enabled()) {
         if (kvmppc_enable_cap_large_decr(cpu, val)) {
-            error_setg(errp,
-                       "No large decrementer support,"
-                       " try appending -machine cap-large-decr=off");
+            error_setg(errp, "No large decrementer support");
+            error_append_hint(errp,
+                              "Try appending -machine cap-large-decr=off\n");
         }
     }
 
@@ -482,6 +493,7 @@ static void cap_large_decr_cpu_apply(SpaprMachineState *spapr,
 static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val,
                                  Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     uint8_t kvm_val = kvmppc_get_cap_count_cache_flush_assist();
 
     if (tcg_enabled() && val) {
@@ -504,14 +516,15 @@ static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val,
             return;
         }
         error_setg(errp,
-"Requested count cache flush assist capability level not supported by kvm,"
-                   " try appending -machine cap-ccf-assist=off");
+                   "Requested count cache flush assist capability level not supported by KVM");
+        error_append_hint(errp, "Try appending -machine cap-ccf-assist=off\n");
     }
 }
 
 static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
                                 Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     if (!val) {
         return; /* Disabled by default */
     }

[PATCH v2 3/3] spapr: Forbid nested KVM-HV in pre-power9 compat mode

[Greg Kurz]

Nested KVM-HV only works on POWER9.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_caps.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 2cb7ba8f005a..e86a7c350444 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -409,6 +409,8 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
                                     uint8_t val, Error **errp)
 {
     ERRP_AUTO_PROPAGATE();
+    PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
+
     if (!val) {
         /* capability disabled by default */
         return;
@@ -418,6 +420,14 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
         error_setg(errp, "No Nested KVM-HV support in TCG");
         error_append_hint(errp, "Try appending -machine cap-nested-hv=off\n");
     } else if (kvm_enabled()) {
+        if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_00, 0,
+                              spapr->max_compat_pvr)) {
+            error_setg(errp, "Nested KVM-HV only supported on POWER9");
+            error_append_hint(errp,
+                              "Try appending -machine max-cpu-compat=power9\n");
+            return;
+        }
+
         if (!kvmppc_has_cap_nested_kvm_hv()) {
             error_setg(errp,
                        "KVM implementation does not support Nested KVM-HV");

Re: [PATCH v2 3/3] spapr: Forbid nested KVM-HV in pre-power9 compat mode

[Laurent Vivier]

On 11/06/2020 11:10, Greg Kurz wrote:
> Nested KVM-HV only works on POWER9.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr_caps.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 2cb7ba8f005a..e86a7c350444 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -409,6 +409,8 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
>                                      uint8_t val, Error **errp)
>  {
>      ERRP_AUTO_PROPAGATE();
> +    PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> +
>      if (!val) {
>          /* capability disabled by default */
>          return;
> @@ -418,6 +420,14 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
>          error_setg(errp, "No Nested KVM-HV support in TCG");
>          error_append_hint(errp, "Try appending -machine cap-nested-hv=off\n");
>      } else if (kvm_enabled()) {
> +        if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_00, 0,
> +                              spapr->max_compat_pvr)) {
> +            error_setg(errp, "Nested KVM-HV only supported on POWER9");
> +            error_append_hint(errp,
> +                              "Try appending -machine max-cpu-compat=power9\n");
> +            return;
> +        }
> +
>          if (!kvmppc_has_cap_nested_kvm_hv()) {
>              error_setg(errp,
>                         "KVM implementation does not support Nested KVM-HV");
> 
> 

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Re: [PATCH v2 1/3] error: auto propagated local_err

[Laurent Vivier]

On 11/06/2020 11:09, Greg Kurz wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Introduce a new ERRP_AUTO_PROPAGATE macro, to be used at start of
> functions with an errp OUT parameter.
> 
> It has three goals:
> 
> 1. Fix issue with error_fatal and error_prepend/error_append_hint: user
> can't see this additional information, because exit() happens in
> error_setg earlier than information is added. [Reported by Greg Kurz]
> 
> 2. Fix issue with error_abort and error_propagate: when we wrap
> error_abort by local_err+error_propagate, the resulting coredump will
> refer to error_propagate and not to the place where error happened.
> (the macro itself doesn't fix the issue, but it allows us to [3.] drop
> the local_err+error_propagate pattern, which will definitely fix the
> issue) [Reported by Kevin Wolf]
> 
> 3. Drop local_err+error_propagate pattern, which is used to workaround
> void functions with errp parameter, when caller wants to know resulting
> status. (Note: actually these functions could be merely updated to
> return int error code).
> 
> To achieve these goals, later patches will add invocations
> of this macro at the start of functions with either use
> error_prepend/error_append_hint (solving 1) or which use
> local_err+error_propagate to check errors, switching those
> functions to use *errp instead (solving 2 and 3).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Paul Durrant <paul@xen.org>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  include/qapi/error.h |  205 ++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 173 insertions(+), 32 deletions(-)

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

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

[Vladimir Sementsov-Ogievskiy]

11.06.2020 12: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>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

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

[Laurent Vivier]

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

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

[Greg Kurz]

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.

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

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

[Vladimir Sementsov-Ogievskiy]

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

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

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

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;
> +        }

what a stupid code :) at least, if condition is always true.

this all should be substitute by just

	warn_report("TCG doesn't support requested feature, cap-cfpc=%s", cap_cfpc_possible.vals[val]);


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

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

[Greg Kurz]

On Thu, 11 Jun 2020 13:42:48 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 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;
> > +        }
> 
> what a stupid code :) at least, if condition is always true.
> 
> this all should be substitute by just
> 
> 	warn_report("TCG doesn't support requested feature, cap-cfpc=%s", cap_cfpc_possible.vals[val]);
> 

Exactly ! :)

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

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

[Vladimir Sementsov-Ogievskiy]

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


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

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

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

[Laurent Vivier]

On 11/06/2020 12:44, Greg Kurz wrote:
> On Thu, 11 Jun 2020 13:42:48 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
>> 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;
>>> +        }
>>
>> what a stupid code :) at least, if condition is always true.
>>
>> this all should be substitute by just
>>
>> 	warn_report("TCG doesn't support requested feature, cap-cfpc=%s", cap_cfpc_possible.vals[val]);
>>
> 
> Exactly ! :)
> 

Yes, it seems appropriate.

Thanks,
Laurent