diff mbox series

[v3,12/13] block/qcow2: simplify qcow2_co_invalidate_cache()

Message ID 20201016171057.6182-13-vsementsov@virtuozzo.com
State Accepted
Commit e6247c9c9f9e4d01b00036f017da53d130981727
Headers show
Series block: deal with errp: part I | expand

Commit Message

Vladimir Sementsov-Ogievskiy Oct. 16, 2020, 5:10 p.m. UTC 
qcow2_do_open correctly sets errp on each failure path. So, we can
simplify code in qcow2_co_invalidate_cache() and drop explicit error
propagation.

Add ERRP_GUARD() as mandated by the documentation in
include/qapi/error.h so that error_prepend() is actually called even if
errp is &error_fatal.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
 block/qcow2.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

Comments

Kevin Wolf Oct. 19, 2020, 1:06 p.m. UTC | #1 
Am 16.10.2020 um 19:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
> qcow2_do_open correctly sets errp on each failure path. So, we can

> simplify code in qcow2_co_invalidate_cache() and drop explicit error

> propagation.


qcow2_update_options_prepare() can return -EINVAL without setting errp:

    if (s->crypt_method_header != QCOW_CRYPT_NONE && !r->crypto_opts) {
        ret = -EINVAL;
        goto fail;
    }

This is called indirectly from qcow2_do_open().

Kevin
Vladimir Sementsov-Ogievskiy Oct. 20, 2020, 12:46 p.m. UTC | #2 
19.10.2020 16:06, Kevin Wolf wrote:
> Am 16.10.2020 um 19:10 hat Vladimir Sementsov-Ogievskiy geschrieben:

>> qcow2_do_open correctly sets errp on each failure path. So, we can

>> simplify code in qcow2_co_invalidate_cache() and drop explicit error

>> propagation.

> 

> qcow2_update_options_prepare() can return -EINVAL without setting errp:

> 

>      if (s->crypt_method_header != QCOW_CRYPT_NONE && !r->crypto_opts) {

>          ret = -EINVAL;

>          goto fail;

>      }

> 


Actually, it's set in previous "switch", either in "default:" branch or if block_crypto_open_opts_init() failed.. But the code pattern is far from being obvious, I'll add a patch to refactor.


-- 
Best regards,
Vladimir
diff mbox series

Patch

diff --git a/block/qcow2.c b/block/qcow2.c
index 2b6ec4b757..cd5f48d3fb 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2702,11 +2702,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;
 
     /*
@@ -2724,16 +2724,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;
     }