Re: [PATCH 02/46] error: Document Error API usage rules

[Greg Kurz]

On Wed, 24 Jun 2020 18:43:00 +0200
Markus Armbruster <armbru@redhat.com> 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.
> 

This confusion is reinforced by the fact that the standard pattern:

    error_setg(errp, ...);
    error_append_hint(errp, ...);

doesn't work when errp is &error_fatal, which is a typical case of
an invalid command line argument, where it is valuable to suggest
something sensible to the user but error_setg() exits before we
could do so.

Fortunately, Vladimir's work will address that and eliminate the
temptation to workaround the issue with more boilerplate :)

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

Reviewed-by: Greg Kurz <groug@kaod.org>

>  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.
> + *
> + * - 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");
>   *