[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 03/45] error: Document Error API usage rules
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v4 03/45] error: Document Error API usage rules |
Date: |
Tue, 07 Jul 2020 21:23:32 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
Eric Blake <eblake@redhat.com> writes:
> 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/
Yes.
>> + * 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,
Yes, there are such functions.
> 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:
What about
- when it does not, say 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);
>>
Thanks!
- [PATCH v4 00/45] Less clumsy error checking, Markus Armbruster, 2020/07/07
- [PATCH v4 11/45] qemu-option: Factor out helper opt_create(), Markus Armbruster, 2020/07/07
- [PATCH v4 08/45] qemu-option: Make uses of find_desc_by_name() more similar, Markus Armbruster, 2020/07/07
- [PATCH v4 04/45] qdev: Use returned bool to check for qdev_realize() etc. failure, Markus Armbruster, 2020/07/07
- [PATCH v4 20/45] s390x/pci: Fix harmless mistake in zpci's property fid's setter, Markus Armbruster, 2020/07/07
- [PATCH v4 15/45] block: Avoid error accumulation in bdrv_img_create(), Markus Armbruster, 2020/07/07
- [PATCH v4 10/45] qemu-option: Simplify around find_default_by_name(), Markus Armbruster, 2020/07/07
- [PATCH v4 09/45] qemu-option: Factor out helper find_default_by_name(), Markus Armbruster, 2020/07/07
- [PATCH v4 06/45] virtio-crypto-pci: Tidy up virtio_crypto_pci_realize(), Markus Armbruster, 2020/07/07