17.09.2020 19:23, Alberto Garcia wrote:
On Wed 09 Sep 2020 08:59:29 PM CEST, Vladimir Sementsov-Ogievskiy
<vsement...@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 <vsement...@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.
--
Best regards,
Vladimir
