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

[Vladimir Sementsov-Ogievskiy]

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