25.06.2020 14:23, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
>> 24.06.2020 19:43, Markus Armbruster wrote:
>>> This merely codifies existing practice, with one exception: the rule
>>> advising against returning void, where existing practice is mixed.
>>>
>>> When the Error API was created, we adopted the (unwritten) rule to
>>> return void when the function returns no useful value on success,
>>> unlike GError, which recommends to return true on success and false on
>>> error then.
>>>
>>> When a function returns a distinct error value, say false, a checked
>>> call that passes the error up looks like
>>>
>>> if (!frobnicate(..., errp)) {
>>> handle the error...
>>> }
>>>
>>> When it returns void, we need
>>>
>>> Error *err = NULL;
>>>
>>> frobnicate(..., &err);
>>> if (err) {
>>> handle the error...
>>> error_propagate(errp, err);
>>> }
>>>
>>> Not only is this more verbose, it also creates an Error object even
>>> when @errp is null, &error_abort or &error_fatal.
>>>
>>> People got tired of the additional boilerplate, and started to ignore
>>> the unwritten rule. The result is confusion among developers about
>>> the preferred usage.
>>>
>>> The written rule will hopefully reduce the confusion.
>>>
>>> The remainder of this series will update a substantial amount of code
>>> to honor the rule.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>> include/qapi/error.h | 26 ++++++++++++++++++++++++++
>>> 1 file changed, 26 insertions(+)
>>>
>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>> index 1a5ea25e12..c3d84d610a 100644
>>> --- a/include/qapi/error.h
>>> +++ b/include/qapi/error.h
>>> @@ -15,6 +15,32 @@
>>> /*
>>> * Error reporting system loosely patterned after Glib's GError.
>>> *
>>> + * Rules:
>>> + *
>>> + * - Functions that use Error to report errors have an Error **errp
>>> + * parameter. It should be the last parameter, except for functions
>>> + * taking variable arguments.
>>> + *
>>> + * - You may pass NULL to not receive the error, &error_abort to abort
>>> + * on error, &error_fatal to exit(1) on error, or a pointer to a
>>> + * variable containing NULL to receive the error.
>>> + *
>>> + * - The value of @errp should not affect control flow.
>>
>> What do you mean? Incoming state of errp, or *errp after some call of another
>> function? Should we then update this paragraph, when introduce
>> ERRP_AUTO_PROPAGATE?
>
> The argument value passed for parameter @errp.
>
> What I'm trying to express is that the function should remain oblivious
> of how the caller handles errors. Do not check whether the argument is
> NULL, &error_abort, &error_fatal, or any other value. It's best to
> treat @errp as write-only.
>
> I'm trying to strike a balance between clarity and brevity, without
> overspecifying what the function may do. I tend to err on the side of
> brevity in function contracts. I always hope reviewers will flag my
> errors :) In short, I'm open to better ideas.
>
> GLib documentation, for comparison:
>
> If NULL is passed for the GError** argument, then errors should not
> be returned to the caller, but your function should still abort and
> return if an error occurs. That is, control flow should not be
> affected by whether the caller wants to get a GError.
>
Ah, OK. I'm about the fact that ERRP_AUTO_PROPAGATE exactly do some things based on
the value of errp. Still it's not about general function code-flow, but special
error-reporting related magic. (as well as error_setg will rely on errp value too,
but it's not related to actual function code-flow).
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>>> + *
>>> + * - On success, the function should not use @errp. On failure, it
>>> + * should set a new error, e.g. with error_setg(errp, ...), or
>>> + * propagate an existing one, e.g. with error_propagate(errp, ...).
>>> + *
>>> + * - Whenever practical, also return a value that indicates success /
>>> + * failure. This can make the error checking more concise, and can
>>> + * avoid useless error object creation and destruction. Note that
>>> + * we still have many functions returning void. We recommend
>>> + * • bool-valued functions return true on success / false on failure,
>>> + * • pointer-valued functions return non-null / null pointer, and
>>> + * • integer-valued functions return non-negative / negative.
>>> + *
>>> + * How to:
>>> + *
>>> * Create an error:
>>> * error_setg(errp, "situation normal, all fouled up");
>>> *
>>>
>
--
Best regards,
Vladimir