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

[Vladimir Sementsov-Ogievskiy]

12.01.2021 00:17, Vladimir Sementsov-Ogievskiy wrote:
> 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.

Which is actually done during the series, so there is no local_err ;)

> > 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