diff mbox series

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

Message ID 20200909185930.26524-14-vsementsov@virtuozzo.com
State New
Headers show
Series block: deal with errp: part I | expand

Commit Message

Vladimir Sementsov-Ogievskiy Sept. 9, 2020, 6:59 p.m. UTC 
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(-)

Comments

Alberto Garcia Sept. 17, 2020, 4:23 p.m. UTC | #1 
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
diff mbox series

Patch

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