Re: [PATCH v4 03/45] error: Document Error API usage rules

[Eric Blake <eblake@redhat.com>]

On 7/7/20 11:05 AM, 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.
> 

> Make the rule advising against returning void official by putting it
> in writing.  This will hopefully reduce confusion.
> 
> Update the examples accordingly.
> 
> 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: Eric Blake <eblake@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> ---

> @@ -95,14 +122,12 @@
>    * Create a new error and pass it to the caller:
>    *     error_setg(errp, "situation normal, all fouled up");
>    *
> - * Call a function and receive an error from it:
> - *     Error *err = NULL;
> - *     foo(arg, &err);
> - *     if (err) {
> + * Call a function, receive an error from it, and pass it to caller

maybe s/to caller/to the caller/

> + * when the function returns a value that indicates failure, say false:
> + *     if (!foo(arg, errp)) {
>    *         handle the error...
>    *     }
> - *
> - * Receive an error and pass it on to the caller:
> + * when it doesn't, say a void function:

Hmm. It looks like you have a single sentence "Call a function... when 
the function returns", but this line now makes it obvious that you have 
a single prefix: "Call a function, ...and pass it to the caller:"
with two choices "when the function returns" and "when it doesn't".  I'm 
not sure if there is a nicer way to typeset it, adding yet another ":" 
at the end of the line looks odd.  The idea behind the text is fine, I'm 
just trying to paint the bikeshed to see if there is a better presentation.

>    *     Error *err = NULL;
>    *     foo(arg, &err);
>    *     if (err) {
> @@ -120,6 +145,19 @@
>    *     foo(arg, errp);
>    * for readability.
>    *
> + * Receive an error, and handle it locally
> + * when the function returns a value that indicates failure, say false:
> + *     Error *err = NULL;
> + *     if (!foo(arg, &err)) {
> + *         handle the error...
> + *     }
> + * when it doesn't, say a void function:

It helps that you have repeated the same pattern as above.  But that 
means if you change the layout, both groupings should have the same 
layout.  Maybe:

Intro for a task:
- when the function returns...
- when it doesn't

Also, are there functions that have a return type other than void, but 
where the return value is not an indication of error?  If there are, 
then the "say a void function" clause makes sense (but we should 
probably recommend against such functions); if there are not, then "say 
a void function" reads awkwardly.  Maybe:

- when it does not, because it is a void function:

> + *     Error *err = NULL;
> + *     foo(arg, &err);
> + *     if (err) {
> + *         handle the error...
> + *     }
> + *
>    * Receive and accumulate multiple errors (first one wins):
>    *     Error *err = NULL, *local_err = NULL;
>    *     foo(arg, &err);
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org