|
From: | Vladimir Sementsov-Ogievskiy |
Subject: | Re: [PATCH v2 2/3] spapr: Use error_append_hint() in spapr_caps.c |
Date: | Thu, 11 Jun 2020 13:44:25 +0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.1 |
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
[Prev in Thread] | Current Thread | [Next in Thread] |