[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 13/14] block/qcow2: qcow2_do_open: deal with errp

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 &
+        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 

Thanks for review! I'll post v2 soon.

Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]