| Message ID | 20200909185930.26524-14-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: deal with errp: part I | expand |
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
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.
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(); 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"); ret = -ENOTSUP; g_free(feature_table); goto fail; @@ -2709,11 +2712,11 @@ static void qcow2_close(BlockDriverState *bs) static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs, Error **errp) { + ERRP_GUARD(); BDRVQcow2State *s = bs->opaque; int flags = s->flags; QCryptoBlock *crypto = NULL; QDict *options; - Error *local_err = NULL; int ret; /* @@ -2731,16 +2734,11 @@ static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs, flags &= ~BDRV_O_INACTIVE; qemu_co_mutex_lock(&s->lock); - ret = qcow2_do_open(bs, options, flags, &local_err); + ret = qcow2_do_open(bs, options, flags, errp); qemu_co_mutex_unlock(&s->lock); qobject_unref(options); - if (local_err) { - error_propagate_prepend(errp, local_err, - "Could not reopen qcow2 layer: "); - bs->drv = NULL; - return; - } else if (ret < 0) { - error_setg_errno(errp, -ret, "Could not reopen qcow2 layer"); + if (ret < 0) { + error_prepend(errp, "Could not reopen qcow2 layer: "); bs->drv = NULL; return; }
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(-)