Re: [PATCH v2 10/13] block/qcow2-bitmap: return status from qcow2_store_persistent_dirty_bitmaps

[Alberto Garcia]

On Thu 17 Sep 2020 09:55:16 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> It's better to return status together with setting errp. It makes
> possible to avoid error propagation.
>
> While being here, put ERRP_GUARD() to fix error_prepend(errp, ...)
> usage inside qcow2_store_persistent_dirty_bitmaps() (see the comment
> above ERRP_GUARD() definition in include/qapi/error.h)
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> Reviewed-by: Greg Kurz 

Reviewed-by: Alberto Garcia 

Berto

[PATCH v2 10/13] block/qcow2-bitmap: return status from qcow2_store_persistent_dirty_bitmaps

[Vladimir Sementsov-Ogievskiy]

It's better to return status together with setting errp. It makes
possible to avoid error propagation.

While being here, put ERRP_GUARD() to fix error_prepend(errp, ...)
usage inside qcow2_store_persistent_dirty_bitmaps() (see the comment
above ERRP_GUARD() definition in include/qapi/error.h)

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Greg Kurz 
---
 block/qcow2.h|  2 +-
 block/qcow2-bitmap.c | 13 ++---
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 3c64dcda33..7884a5088d 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -978,7 +978,7 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
 Qcow2BitmapInfoList **info_list, Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
 int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
   bool release_stored, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
 bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 500175f4e8..b8ff347885 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1534,9 +1534,10 @@ out:
  * readonly to begin with, and whether we opened directly or reopened to that
  * state shouldn't matter for the state we get afterward.
  */
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
   bool release_stored, Error **errp)
 {
+ERRP_GUARD();
 BdrvDirtyBitmap *bitmap;
 BDRVQcow2State *s = bs->opaque;
 uint32_t new_nb_bitmaps = s->nb_bitmaps;
@@ -1556,7 +1557,7 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
 bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
s->bitmap_directory_size, errp);
 if (bm_list == NULL) {
-return;
+return false;
 }
 }
 
@@ -1671,7 +1672,7 @@ success:
 }
 
 bitmap_list_free(bm_list);
-return;
+return true;
 
 fail:
 QSIMPLEQ_FOREACH(bm, bm_list, entry) {
@@ -1689,16 +1690,14 @@ fail:
 }
 
 bitmap_list_free(bm_list);
+return false;
 }
 
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
 {
 BdrvDirtyBitmap *bitmap;
-Error *local_err = NULL;
 
-qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err);
-if (local_err != NULL) {
-error_propagate(errp, local_err);
+if (!qcow2_store_persistent_dirty_bitmaps(bs, false, errp)) {
 return -EINVAL;
 }
 
-- 
2.21.3