|
From: | Vladimir Sementsov-Ogievskiy |
Subject: | Re: [PATCH 13/14] block/qcow2: qcow2_do_open: deal with errp |
Date: | Thu, 17 Sep 2020 20:44:04 +0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.2.2 |
17.09.2020 19:23, Alberto Garcia wrote:
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?
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
[Prev in Thread] | Current Thread | [Next in Thread] |