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

[Eric Blake]

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

signature.asc
Description: OpenPGP digital signature