Re: [PATCH v5 14/14] block/qcow2: refactor qcow2_update_options_prepare error paths
12.01.2021 00:17, Vladimir Sementsov-Ogievskiy wrote:
11.01.2021 19:08, Alberto Garcia wrote:
On Sat 09 Jan 2021 01:58:11 PM CET, Vladimir Sementsov-Ogievskiy wrote:
Keep setting ret close to setting errp and don't merge different error
paths into one. This way it's more obvious that we don't return
error without setting errp.
Signed-off-by: Vladimir Sementsov-Ogievskiy
I get the idea, but I feel the code is getting innecessarily verbose.
One alternative would be to get rid of all -EINVAL inside the switch
block, take advantage of the existing local_err and follow the block
with:
if (local_err) {
error_propagate(errp, local_err);
ret = -EINVAL;
goto fail;
}
Actually in our new paradigm of avoiding error propagation (as well as void
functions with errp argument), we should first update read_cache_sizes() to
return the value together with setting errp, then we will update
read_cache_sizes() call in qcow2_update_options_prepare() and drop local_err
from qcow2_update_options_prepare() at all.
Which is actually done during the series, so there is no local_err ;)
But otherwise your solution is correct, so you can keep it if you
prefer:
Reviewed-by: Alberto Garcia
Thanks!
--
Best regards,
Vladimir
Re: [PATCH v5 14/14] block/qcow2: refactor qcow2_update_options_prepare error paths
11.01.2021 19:08, Alberto Garcia wrote:
On Sat 09 Jan 2021 01:58:11 PM CET, Vladimir Sementsov-Ogievskiy wrote:
Keep setting ret close to setting errp and don't merge different error
paths into one. This way it's more obvious that we don't return
error without setting errp.
Signed-off-by: Vladimir Sementsov-Ogievskiy
I get the idea, but I feel the code is getting innecessarily verbose.
One alternative would be to get rid of all -EINVAL inside the switch
block, take advantage of the existing local_err and follow the block
with:
if (local_err) {
error_propagate(errp, local_err);
ret = -EINVAL;
goto fail;
}
Actually in our new paradigm of avoiding error propagation (as well as void
functions with errp argument), we should first update read_cache_sizes() to
return the value together with setting errp, then we will update
read_cache_sizes() call in qcow2_update_options_prepare() and drop local_err
from qcow2_update_options_prepare() at all.
But otherwise your solution is correct, so you can keep it if you
prefer:
Reviewed-by: Alberto Garcia
Thanks!
--
Best regards,
Vladimir
Re: [PATCH v5 14/14] block/qcow2: refactor qcow2_update_options_prepare error paths
On Sat 09 Jan 2021 01:58:11 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> Keep setting ret close to setting errp and don't merge different error
> paths into one. This way it's more obvious that we don't return
> error without setting errp.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
I get the idea, but I feel the code is getting innecessarily verbose.
One alternative would be to get rid of all -EINVAL inside the switch
block, take advantage of the existing local_err and follow the block
with:
if (local_err) {
error_propagate(errp, local_err);
ret = -EINVAL;
goto fail;
}
But otherwise your solution is correct, so you can keep it if you
prefer:
Reviewed-by: Alberto Garcia
Berto
[PATCH v5 14/14] block/qcow2: refactor qcow2_update_options_prepare error paths
Keep setting ret close to setting errp and don't merge different error
paths into one. This way it's more obvious that we don't return
error without setting errp.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/qcow2.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 436bcf0a97..0aa6ae1e1f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1158,6 +1158,10 @@ static int qcow2_update_options_prepare(BlockDriverState
*bs,
}
qdict_put_str(encryptopts, "format", "qcow");
r->crypto_opts = block_crypto_open_opts_init(encryptopts, errp);
+if (!r->crypto_opts) {
+ret = -EINVAL;
+goto fail;
+}
break;
case QCOW_CRYPT_LUKS:
@@ -1170,14 +1174,15 @@ static int
qcow2_update_options_prepare(BlockDriverState *bs,
}
qdict_put_str(encryptopts, "format", "luks");
r->crypto_opts = block_crypto_open_opts_init(encryptopts, errp);
+if (!r->crypto_opts) {
+ret = -EINVAL;
+goto fail;
+}
break;
default:
error_setg(errp, "Unsupported encryption method %d",
s->crypt_method_header);
-break;
-}
-if (s->crypt_method_header != QCOW_CRYPT_NONE && !r->crypto_opts) {
ret = -EINVAL;
goto fail;
}
--
2.29.2
