Re: [PATCH v5 14/14] block/qcow2: refactor qcow2_update_options_prepare error paths

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

11.01.2021 19:08, Alberto Garcia wrote:
> On Sat 09 Jan 2021 01:58:11 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>> Keep setting ret close to setting errp and don't merge different error
>> paths into one. This way it's more obvious that we don't return
>> error without setting errp.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> I get the idea, but I feel the code is getting innecessarily verbose.
> 
> One alternative would be to get rid of all -EINVAL inside the switch
> block, take advantage of the existing local_err and follow the block
> with:
> 
>      if (local_err) {
>          error_propagate(errp, local_err);
>          ret = -EINVAL;
>          goto fail;
>      }

Actually in our new paradigm of avoiding error propagation (as well as void functions with errp argument), we should first update read_cache_sizes() to return the value together with setting errp, then we will update read_cache_sizes() call in qcow2_update_options_prepare() and drop local_err from qcow2_update_options_prepare() at all.

> 
> But otherwise your solution is correct, so you can keep it if you
> prefer:
> 
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> 

Thanks!


-- 
Best regards,
Vladimir