Re: [Qemu-devel] [PATCH for-2.5] error: Document chained error handling

[Markus Armbruster <armbru@redhat.com>]

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?