[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial v2] block.c: Add return v
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial v2] block.c: Add return value for bdrv_append_temp_snapshot() to avoid incorrect failure processing issue |
Date: |
Tue, 24 Jun 2014 13:01:52 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
Kevin Wolf <address@hidden> 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 <address@hidden>
>
> 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.
> 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;
}