On Wed 09 Sep 2020 08:59:29 PM CEST, Vladimir Sementsov-Ogievskiy
<vsement...@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 <vsement...@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
