Re: [PATCH v4 for-5.2 1/2] spapr: Use error_append_hint() in spapr_caps.c

[Markus Armbruster <armbru@redhat.com>]

Greg Kurz <groug@kaod.org> writes:

> On Mon, 20 Jul 2020 17:24:35 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Greg Kurz <groug@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.

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

>> No need to make an argument involving the possible arguments (pardon the
>> pun).
>> 
>
> :)
>
>> [...]
>>