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

[Alberto Garcia]

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?

>      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();

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

Berto