qemu-trivial
[Top][All Lists]
Advanced

[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: Chen Gang
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: Thu, 26 Jun 2014 06:13:56 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7

Thank you for all of your work, if necessary to send patch v3 for it
(may change the comments), please let me know.

Thanks.

On 06/25/2014 12:00 AM, Michael Tokarev wrote:
> 24.06.2014 15:01, Markus Armbruster wrote:
>> 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.
> 
> 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
> 


-- 
Chen Gang

Open share and attitude like air water and life which God blessed



reply via email to

[Prev in Thread] Current Thread [Next in Thread]