* [Qemu-devel] [PATCH for-2.5] error: Document chained error handling
@ 2015-11-12 18:20 Eric Blake
2015-11-17 13:48 ` Markus Armbruster
0 siblings, 1 reply; 4+ messages in thread
From: Eric Blake @ 2015-11-12 18:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Markus Armbruster, Michael Roth
The current output of the qapi code generator includes some chained
error handling, which looks like:
| visit_start_struct(v, (void **)obj, "foo", name, sizeof(FOO), &err);
| if (!err) {
| if (*obj) {
| visit_type_FOO_fields(v, obj, errp);
| }
| visit_end_struct(v, &err);
| }
| error_propagate(errp, err);
Although there are plans to revisit that code for qemu 2.6, it is
still a useful idiom to mention. It is safe because error_propagate()
is different than most functions in that you can pass in an
already-set errp, and it still does the right thing.
Also, describe an alternative form of chained error handling that was
proposed during the qapi work, and which may be a bit more obvious
to a reader what is happening:
| visit_start_implicit_struct(v, (void **)obj, sizeof(FOO), &err);
| if (!err) {
| visit_type_FOO_fields(v, obj, &err);
| visit_end_implicit_struct(v, err ? NULL : &err);
| }
| error_propagate(errp, err);
Signed-off-by: Eric Blake <eblake@redhat.com>
---
based on feedback of my qapi series v5 7/46; doc only, so might
be worth having in 2.5
---
include/qapi/error.h | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/include/qapi/error.h b/include/qapi/error.h
index 4d42cdc..4310195 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -76,6 +76,21 @@
* But when all you do with the error is pass it on, please use
* foo(arg, errp);
* for readability.
+ *
+ * In a situation where cleanup must happen even if a first step fails,
+ * but the cleanup may also set an error, the first error to occur will
+ * take priority when combined by:
+ * Error *err = NULL;
+ * action1(arg, errp);
+ * action2(arg, &err);
+ * error_propagate(errp, err);
+ * or by:
+ * Error *err = NULL;
+ * action1(arg, &err);
+ * action2(arg, err ? NULL : &err);
+ * error_propagate(errp, err);
+ * although the second form is required if any further error handling
+ * will inspect err to see if all earlier locations succeeded.
*/
#ifndef ERROR_H
--
2.4.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5] error: Document chained error handling
2015-11-12 18:20 [Qemu-devel] [PATCH for-2.5] error: Document chained error handling Eric Blake
@ 2015-11-17 13:48 ` Markus Armbruster
2015-11-17 15:11 ` Eric Blake
0 siblings, 1 reply; 4+ messages in thread
From: Markus Armbruster @ 2015-11-17 13:48 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> The current output of the qapi code generator includes some chained
> error handling, which looks like:
>
> | visit_start_struct(v, (void **)obj, "foo", name, sizeof(FOO), &err);
> | if (!err) {
> | if (*obj) {
> | visit_type_FOO_fields(v, obj, errp);
> | }
> | visit_end_struct(v, &err);
> | }
> | error_propagate(errp, err);
>
> Although there are plans to revisit that code for qemu 2.6, it is
> still a useful idiom to mention. It is safe because error_propagate()
> is different than most functions in that you can pass in an
> already-set errp, and it still does the right thing.
>
> Also, describe an alternative form of chained error handling that was
> proposed during the qapi work, and which may be a bit more obvious
> to a reader what is happening:
>
> | visit_start_implicit_struct(v, (void **)obj, sizeof(FOO), &err);
> | if (!err) {
> | visit_type_FOO_fields(v, obj, &err);
> | visit_end_implicit_struct(v, err ? NULL : &err);
> | }
> | error_propagate(errp, err);
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> based on feedback of my qapi series v5 7/46; doc only, so might
> be worth having in 2.5
> ---
> include/qapi/error.h | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 4d42cdc..4310195 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -76,6 +76,21 @@
> * But when all you do with the error is pass it on, please use
> * foo(arg, errp);
> * for readability.
> + *
> + * In a situation where cleanup must happen even if a first step fails,
> + * but the cleanup may also set an error, the first error to occur will
> + * take priority when combined by:
> + * Error *err = NULL;
> + * action1(arg, errp);
> + * action2(arg, &err);
> + * error_propagate(errp, err);
> + * or by:
> + * Error *err = NULL;
> + * action1(arg, &err);
> + * action2(arg, err ? NULL : &err);
> + * error_propagate(errp, err);
> + * although the second form is required if any further error handling
> + * will inspect err to see if all earlier locations succeeded.
> */
>
> #ifndef ERROR_H
Yet another one:
* Error *err = NULL, *local_err = NULL;
* action1(arg, &err);
* action2(arg, &local_err)
* error_propagate(&err, err);
* error_propagate(errp, err);
Less clever.
Can we find a single, recommended way to do this? See below.
Not mentioned: the obvious
action1(arg, errp);
action2(arg, errp);
is wrong, because a non-null Error ** argument must point to a null. It
isn't when errp is non-null, and action1() sets an error. It actually
fails an assertion in error_setv() when action2() sets an error other
than with error_propagate().
The existing how-to comment doesn't spell this out. It always shows the
required err = NULL, though. You can derive "must point to null" from
the function comments of error_setg() and error_propagate().
I agree the how-to comment could use a section on accumulating errors.
Your patch adds one on "accumulate and pass to caller". Here's my
attempt:
diff --git a/include/qapi/error.h b/include/qapi/error.h
index 4d42cdc..b2362a5 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -76,6 +76,23 @@
* But when all you do with the error is pass it on, please use
* foo(arg, errp);
* for readability.
+ *
+ * Receive and accumulate multiple errors (first one wins):
+ * Error *err = NULL, *local_err = NULL;
+ * foo(arg, &err);
+ * bar(arg, &local_err);
+ * error_propagate(&err, local_err);
+ * if (err) {
+ * handle the error...
+ * }
+ *
+ * Do *not* "optimize" this to
+ * foo(arg, &err);
+ * bar(arg, &err); // WRONG!
+ * if (err) {
+ * handle the error...
+ * }
+ * because this may pass a non-null err to bar().
*/
#ifndef ERROR_H
Leaves replacing &err by errp when the value of err isn't needed to the
reader. I think that's okay given we've shown it already above.
What do you think?
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5] error: Document chained error handling
2015-11-17 13:48 ` Markus Armbruster
@ 2015-11-17 15:11 ` Eric Blake
2015-11-17 15:36 ` Markus Armbruster
0 siblings, 1 reply; 4+ messages in thread
From: Eric Blake @ 2015-11-17 15:11 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 3570 bytes --]
On 11/17/2015 06:48 AM, Markus Armbruster wrote:
>> ---
>> based on feedback of my qapi series v5 7/46; doc only, so might
>> be worth having in 2.5
>> ---
>> + *
>> + * In a situation where cleanup must happen even if a first step fails,
>> + * but the cleanup may also set an error, the first error to occur will
>> + * take priority when combined by:
>> + * Error *err = NULL;
>> + * action1(arg, errp);
>> + * action2(arg, &err);
>> + * error_propagate(errp, err);
Your proposal covers this idiom.
>> + * or by:
>> + * Error *err = NULL;
>> + * action1(arg, &err);
>> + * action2(arg, err ? NULL : &err);
>> + * error_propagate(errp, err);
This idiom doesn't appear in the current code base, so not documenting
it is okay...
>> + * although the second form is required if any further error handling
>> + * will inspect err to see if all earlier locations succeeded.
...if we instead document how to check if either error happened, but
your version also does that.
>> */
>>
>> #ifndef ERROR_H
>
> Yet another one:
>
> * Error *err = NULL, *local_err = NULL;
> * action1(arg, &err);
> * action2(arg, &local_err)
> * error_propagate(&err, err);
This line should be error_propagate(&err, local_err);
> * error_propagate(errp, err);
>
> Less clever.
>
> Can we find a single, recommended way to do this? See below.
>
> Not mentioned: the obvious
>
> action1(arg, errp);
> action2(arg, errp);
>
> is wrong, because a non-null Error ** argument must point to a null. It
> isn't when errp is non-null, and action1() sets an error. It actually
> fails an assertion in error_setv() when action2() sets an error other
> than with error_propagate().
Indeed, pointing out what we must NOT do is worthwhile.
>
> The existing how-to comment doesn't spell this out. It always shows the
> required err = NULL, though. You can derive "must point to null" from
> the function comments of error_setg() and error_propagate().
>
> I agree the how-to comment could use a section on accumulating errors.
> Your patch adds one on "accumulate and pass to caller". Here's my
> attempt:
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 4d42cdc..b2362a5 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -76,6 +76,23 @@
> * But when all you do with the error is pass it on, please use
> * foo(arg, errp);
> * for readability.
> + *
> + * Receive and accumulate multiple errors (first one wins):
> + * Error *err = NULL, *local_err = NULL;
> + * foo(arg, &err);
> + * bar(arg, &local_err);
> + * error_propagate(&err, local_err);
> + * if (err) {
> + * handle the error...
> + * }
> + *
> + * Do *not* "optimize" this to
> + * foo(arg, &err);
> + * bar(arg, &err); // WRONG!
> + * if (err) {
> + * handle the error...
> + * }
> + * because this may pass a non-null err to bar().
I like that.
> */
>
> #ifndef ERROR_H
>
> Leaves replacing &err by errp when the value of err isn't needed to the
> reader. I think that's okay given we've shown it already above.
>
> What do you think?
I agree; knowing when it is safe to replace &err by errp is already
sufficiently covered in existing text, and limiting this example to a
single point is better.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5] error: Document chained error handling
2015-11-17 15:11 ` Eric Blake
@ 2015-11-17 15:36 ` Markus Armbruster
0 siblings, 0 replies; 4+ messages in thread
From: Markus Armbruster @ 2015-11-17 15:36 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 11/17/2015 06:48 AM, Markus Armbruster wrote:
>
>>> ---
>>> based on feedback of my qapi series v5 7/46; doc only, so might
>>> be worth having in 2.5
>>> ---
>
>>> + *
>>> + * In a situation where cleanup must happen even if a first step fails,
>>> + * but the cleanup may also set an error, the first error to occur will
>>> + * take priority when combined by:
>>> + * Error *err = NULL;
>>> + * action1(arg, errp);
>>> + * action2(arg, &err);
>>> + * error_propagate(errp, err);
>
> Your proposal covers this idiom.
>
>>> + * or by:
>>> + * Error *err = NULL;
>>> + * action1(arg, &err);
>>> + * action2(arg, err ? NULL : &err);
>>> + * error_propagate(errp, err);
>
> This idiom doesn't appear in the current code base, so not documenting
> it is okay...
>
>>> + * although the second form is required if any further error handling
>>> + * will inspect err to see if all earlier locations succeeded.
>
> ...if we instead document how to check if either error happened, but
> your version also does that.
>
>>> */
>>>
>>> #ifndef ERROR_H
>>
>> Yet another one:
>>
>> * Error *err = NULL, *local_err = NULL;
>> * action1(arg, &err);
>> * action2(arg, &local_err)
>> * error_propagate(&err, err);
>
> This line should be error_propagate(&err, local_err);
Yes.
>> * error_propagate(errp, err);
>>
>> Less clever.
>>
>> Can we find a single, recommended way to do this? See below.
>>
>> Not mentioned: the obvious
>>
>> action1(arg, errp);
>> action2(arg, errp);
>>
>> is wrong, because a non-null Error ** argument must point to a null. It
>> isn't when errp is non-null, and action1() sets an error. It actually
>> fails an assertion in error_setv() when action2() sets an error other
>> than with error_propagate().
>
> Indeed, pointing out what we must NOT do is worthwhile.
>
>>
>> The existing how-to comment doesn't spell this out. It always shows the
>> required err = NULL, though. You can derive "must point to null" from
>> the function comments of error_setg() and error_propagate().
>>
>> I agree the how-to comment could use a section on accumulating errors.
>> Your patch adds one on "accumulate and pass to caller". Here's my
>> attempt:
>>
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index 4d42cdc..b2362a5 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -76,6 +76,23 @@
>> * But when all you do with the error is pass it on, please use
>> * foo(arg, errp);
>> * for readability.
>> + *
>> + * Receive and accumulate multiple errors (first one wins):
>> + * Error *err = NULL, *local_err = NULL;
>> + * foo(arg, &err);
>> + * bar(arg, &local_err);
>> + * error_propagate(&err, local_err);
>> + * if (err) {
>> + * handle the error...
>> + * }
>> + *
>> + * Do *not* "optimize" this to
>> + * foo(arg, &err);
>> + * bar(arg, &err); // WRONG!
>> + * if (err) {
>> + * handle the error...
>> + * }
>> + * because this may pass a non-null err to bar().
>
> I like that.
>
>> */
>>
>> #ifndef ERROR_H
>>
>> Leaves replacing &err by errp when the value of err isn't needed to the
>> reader. I think that's okay given we've shown it already above.
>>
>> What do you think?
>
> I agree; knowing when it is safe to replace &err by errp is already
> sufficiently covered in existing text, and limiting this example to a
> single point is better.
I'll post it as a formal patch, with your R-by. Thanks!
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-11-17 15:36 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-12 18:20 [Qemu-devel] [PATCH for-2.5] error: Document chained error handling Eric Blake
2015-11-17 13:48 ` Markus Armbruster
2015-11-17 15:11 ` Eric Blake
2015-11-17 15:36 ` Markus Armbruster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.