Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial v2] block.c: Add return value for bdrv_append_temp_snapshot() to avoid incorrect failure processing issue

[Michael Tokarev <mjt@tls.msk.ru>]

24.06.2014 15:01, Markus Armbruster wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 23.06.2014 um 17:28 hat Chen Gang geschrieben:
>>> When failure occurs, 'ret' need be set, or may return 0 to indicate success.
>>> And error_propagate() also need be called only one time within a function.
>>>
>>> It is abnormal to prevent bdrv_append_temp_snapshot() return value but still
>>> set errp when error occurs -- although it contents return value internally.
>>>
>>> So let bdrv_append_temp_snapshot() internal return value outside, and let
>>> all things normal, then fix the issue too.
>>>
>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>
>> What does this fix?
> 
> It fixes the return value of bdrv_open() when
> bdrv_append_temp_snapshot() fails.  Before this patch, it returns a
> positive value, which is wrong.  After the patch, it returns the
> negative error code bdrv_append_temp_snapshot() now returns.

So, what should be done there?   Kevin, maybe you should pick this up
instead of going -trivial route?

>> Having both a return value and an Error* object is duplication and
>> only a sign that a function hasn't been fully converted to the Error
>> framework yet. We shouldn't introduce new instances of this without a
>> very good reason.
> 
> Maybe.  But I very much prefer
> 
>         ret = foo(arg, errp);
>         if (ret < 0) {
>             return ret;
>         }
> 
> over
> 
>         Error *local_err = NULL;
> 
>         foo(arg, &local_err);
>         if (local_err) {
>             error_propagate(errp, local_err);
>             return;
>         }

Yes, this new error propagation is a bit ugly, I dislike it too.

Thanks,

/mjt