On Tue, 28 Jul 2020 09:26:08 +0200
Markus Armbruster <arm...@redhat.com> wrote:
> Greg Kurz <gr...@kaod.org> writes:
>
> > On Mon, 20 Jul 2020 17:24:35 +0200
> > Markus Armbruster <arm...@redhat.com> wrote:
> >
> >> Greg Kurz <gr...@kaod.org> writes:
> >>
> >> > 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_GUARD() macro for error_append_hint() to be
> >> > functional.
> >>
> >> This isn't a request for change in this patch, just an attempt to squash
> >> possible misunderstandings.
> >>
> >> It's true that error_append_hint() without ERRP_GUARD() works as long as
> >> the caller doesn't pass certain errp arguments. But the callee should
> >> work for all possible @errp arguments, not just the ones that get passed
> >> today. That's why error.h wants you to guard *all* uses of
> >> error_append_hint(errp):
> >>
> >> * = Why, when and how to use ERRP_GUARD() =
> >> *
> >> * Without ERRP_GUARD(), use of the @errp parameter is restricted:
> >> * - It must not be dereferenced, because it may be null.
> >> * - It should not be passed to error_prepend() or
> >> * error_append_hint(), because that doesn't work with &error_fatal.
> >> * ERRP_GUARD() lifts these restrictions.
> >>
> >
> > Yeah, I just wanted to emphasize that we were precisely in the case
> > where we _really_ need to lift the restriction, but I'm perfectly fine
> > with dropping this sentence if you consider it useless.
>
> I lean towards dropping it.
>
David,
Do you want me to send an updated version of this patch or can you
fix this in your tree ?
> > BTW, should we have a way for CI to ensure that a patch that adds
> > error_prepend(errp, ...) or error_append_hint(errp, ...) also adds
> > ERRP_GUARD() ? Not sure that people read error.h that often...
>
> I don't know. Wait and see whether it's worth automating? We didn't
> automate checking other Error API rules, like "no newlines in error
> messages". That one can't crash, though.
>
> The check would have to look beyond the patch, which checkpatch.pl
> doesn't do.
>
<thinking aloud>
Maybe checkpatch.pl could be fed with an extended version of the patch
that has enough context, eg. git show -U$(wc -l ${file}) ${file} ?
</thinking aloud>
> >> No need to make an argument involving the possible arguments (pardon the
> >> pun).
> >>
> >
> > :)
> >
> >> [...]
> >>
>
