Re: [PATCH 13/14] block/qcow2: qcow2_do_open: deal with errp

[Vladimir Sementsov-Ogievskiy]

17.09.2020 19:23, Alberto Garcia wrote:
> On Wed 09 Sep 2020 08:59:29 PM CEST, Vladimir Sementsov-Ogievskiy 
> <vsementsov@virtuozzo.com> wrote:
> > 1. Drop extra error propagation
> >
> > 2. Set errp always on failure. Generic bdrv_open_driver supports driver
> > functions which can return negative value and forget to set errp.
> > That's a strange thing.. Let's improve qcow2_do_open to not behave this
> > way. This allows to simplify code in qcow2_co_invalidate_cache().
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   block/qcow2.c | 16 +++++++---------
> >   1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 31dd28d19e..cc4e7dd461 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -1292,6 +1292,7 @@ static int validate_compression_type(BDRVQcow2State *s, 
> > Error **errp)
> >   static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
> >                                         int flags, Error **errp)
> >   {
> > +    ERRP_GUARD();
>
> Why is this necessary?

Because error_append_hint() used in the function. Without ERRP_GUARD, 
error_append_hint won't work if errp = &error_fatal
Read more in include/qapi/error.h near ERRP_GUARD definition.

But yes, it's good to not it in commit message.

> >       BDRVQcow2State *s = bs->opaque;
> >       unsigned int len, i;
> >       int ret = 0;
> > @@ -1426,6 +1427,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
> > *bs, QDict *options,
> >           report_unsupported_feature(errp, feature_table,
> >                                      s->incompatible_features &
> >                                      ~QCOW2_INCOMPAT_MASK);
> > +        error_setg(errp,
> > +                   "qcow2 header contains unknown
> >       incompatible_feature bits");
>
> I think that this is a mistake because the previous call to
> report_unsupported_feature() already calls error_setg();

Oops, you are right.

> > @@ -2709,11 +2712,11 @@ static void qcow2_close(BlockDriverState *bs)
> >   static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
> >                                                      Error **errp)
> >   {
> > +    ERRP_GUARD();
>
> Again, why is this necessary?

Because it uses error_prepend() after conversion (same reason as for 
error_append_hint()).

Thanks for review! I'll post v2 soon.

--
Best regards,
Vladimir