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

[Vladimir Sementsov-Ogievskiy]

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

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

[Kevin Wolf]

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

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

[Vladimir Sementsov-Ogievskiy]

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 _fatal.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Alberto Garcia 
Reviewed-by: Greg Kurz 
---
 block/qcow2.c | 13 -
 1 file changed, 4 insertions(+), 9 deletions(-)

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(>lock);
-ret = qcow2_do_open(bs, options, flags, _err);
+ret = qcow2_do_open(bs, options, flags, errp);
 qemu_co_mutex_unlock(>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;
 }
-- 
2.21.3