Eric Blake <ebl...@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!
