| Message ID | 20200707160613.848843-4-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Less clumsy error checking | expand |
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 <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!
On 7/7/20 2:23 PM, Markus Armbruster wrote: >> 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: Reasonable.
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);
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(-)