On Thu, 11 Jun 2020 13:42:48 +0300
Vladimir Sementsov-Ogievskiy <vsement...@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 <lviv...@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 <gr...@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?
> >
>
>
