qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Markus Armbruster <armbru@redhat.com>
To: qemu-devel@nongnu.org
Cc: peter.maydell@linaro.org, vsementsov@virtuozzo.com,
	berrange@redhat.com, ehabkost@redhat.com, qemu-block@nongnu.org,
	groug@kaod.org, pbonzini@redhat.com
Subject: [PATCH v4 03/45] error: Document Error API usage rules
Date: Tue,  7 Jul 2020 18:05:31 +0200	[thread overview]
Message-ID: <20200707160613.848843-4-armbru@redhat.com> (raw)
In-Reply-To: <20200707160613.848843-1-armbru@redhat.com>

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.

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>
---
 include/qapi/error.h | 50 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 6 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 6d079c58b7..3fed49747d 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -15,6 +15,33 @@
 /*
  * 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.
+ *
+ * - Separation of concerns: the function is responsible for detecting
+ *   errors and failing cleanly; handling the error is its caller's
+ *   job.  Since the value of @errp is about handling the error, the
+ *   function should not examine it.
+ *
+ * - On success, the function should not touch *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.
+ *
  * = Creating errors =
  *
  * Create an error:
@@ -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
+ * 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:
  *     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:
+ *     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);
-- 
2.26.2



  parent reply	other threads:[~2020-07-07 16:09 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-07 16:05 [PATCH v4 00/45] Less clumsy error checking Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 01/45] error: Fix examples in error.h's big comment Markus Armbruster
2020-07-07 18:35   ` Eric Blake
2020-07-07 16:05 ` [PATCH v4 02/45] error: Improve " Markus Armbruster
2020-07-07 18:38   ` Eric Blake
2020-07-07 19:42   ` Greg Kurz
2020-07-07 16:05 ` Markus Armbruster [this message]
2020-07-07 18:46   ` [PATCH v4 03/45] error: Document Error API usage rules Eric Blake
2020-07-07 19:23     ` Markus Armbruster
2020-07-07 19:47       ` Eric Blake
2020-07-07 16:05 ` [PATCH v4 04/45] qdev: Use returned bool to check for qdev_realize() etc. failure Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 05/45] macio: Tidy up error handling in macio_newworld_realize() Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 06/45] virtio-crypto-pci: Tidy up virtio_crypto_pci_realize() Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 07/45] qemu-option: Check return value instead of @err where convenient Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 08/45] qemu-option: Make uses of find_desc_by_name() more similar Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 09/45] qemu-option: Factor out helper find_default_by_name() Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 10/45] qemu-option: Simplify around find_default_by_name() Markus Armbruster
2020-07-07 19:46   ` Greg Kurz
2020-07-07 16:05 ` [PATCH v4 11/45] qemu-option: Factor out helper opt_create() Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 12/45] qemu-option: Replace opt_set() by cleaner opt_validate() Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 13/45] qemu-option: Make functions taking Error ** return bool, not void Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 14/45] qemu-option: Use returned bool to check for failure Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 15/45] block: Avoid error accumulation in bdrv_img_create() Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 16/45] hmp: Eliminate a variable in hmp_migrate_set_parameter() Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 17/45] qapi: Make visitor functions taking Error ** return bool, not void Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 18/45] qapi: Use returned bool to check for failure, Coccinelle part Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 19/45] qapi: Use returned bool to check for failure, manual part Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 20/45] s390x/pci: Fix harmless mistake in zpci's property fid's setter Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 21/45] qom: Use error_reportf_err() instead of g_printerr() in examples Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 22/45] qom: Rename qdev_get_type() to object_get_type() Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 23/45] qom: Crash more nicely on object_property_get_link() failure Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 24/45] qom: Don't handle impossible " Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 25/45] qom: Use return values to check for error where that's simpler Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 26/45] qom: Put name parameter before value / visitor parameter Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 27/45] qom: Make functions taking Error ** return bool, not void Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 28/45] qom: Use returned bool to check for failure, Coccinelle part Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 29/45] qom: Use returned bool to check for failure, manual part Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 30/45] qom: Make functions taking Error ** return bool, not 0/-1 Markus Armbruster
2020-07-07 16:05 ` [PATCH v4 31/45] qdev: Make functions taking Error ** return bool, not void Markus Armbruster
2020-07-07 16:06 ` [PATCH v4 32/45] qdev: Use returned bool to check for failure, Coccinelle part Markus Armbruster
2020-07-07 16:06 ` [PATCH v4 33/45] error: Avoid unnecessary error_propagate() after error_setg() Markus Armbruster
2020-07-07 16:06 ` [PATCH v4 34/45] error: Eliminate error_propagate() with Coccinelle, part 1 Markus Armbruster
2020-07-07 16:06 ` [PATCH v4 35/45] error: Eliminate error_propagate() with Coccinelle, part 2 Markus Armbruster
2020-07-07 16:06 ` [PATCH v4 36/45] error: Eliminate error_propagate() manually Markus Armbruster
2020-07-07 16:06 ` [PATCH v4 37/45] error: Reduce unnecessary error propagation Markus Armbruster
2020-07-07 16:06 ` [PATCH v4 38/45] block/parallels: Simplify parallels_open() after previous commit Markus Armbruster
2020-07-07 16:06 ` [PATCH v4 39/45] qapi: Smooth another visitor error checking pattern Markus Armbruster
2020-07-07 16:06 ` [PATCH v4 40/45] qapi: Smooth visitor error checking in generated code Markus Armbruster
2020-07-07 16:06 ` [PATCH v4 41/45] qapi: Purge error_propagate() from QAPI core Markus Armbruster
2020-07-07 16:06 ` [PATCH v4 42/45] error: Avoid error_propagate() after migrate_add_blocker() Markus Armbruster
2020-07-07 16:06 ` [PATCH v4 43/45] qemu-img: Ignore Error objects where the return value suffices Markus Armbruster
2020-07-07 16:06 ` [PATCH v4 44/45] qdev: " Markus Armbruster
2020-07-07 16:06 ` [PATCH v4 45/45] hmp: " Markus Armbruster
2020-07-07 16:27 ` [PATCH v4 00/45] Less clumsy error checking Markus Armbruster
2020-07-07 17:10 ` no-reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200707160613.848843-4-armbru@redhat.com \
    --to=armbru@redhat.com \
    --cc=berrange@redhat.com \
    --cc=ehabkost@redhat.com \
    --cc=groug@kaod.org \
    --cc=pbonzini@redhat.com \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=vsementsov@virtuozzo.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).