diff mbox series

[10/14] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface

Message ID 20200909185930.26524-11-vsementsov@virtuozzo.com
State Superseded
Headers show
Series block: deal with errp: part I | expand

Commit Message

Vladimir Sementsov-Ogievskiy Sept. 9, 2020, 6:59 p.m. UTC
It's recommended for bool functions with errp to return true on success
and false on failure. Non-standard interfaces don't help to understand
the code. The change is also needed to reduce error propagation.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.h        |  3 ++-
 block/qcow2-bitmap.c | 22 +++++++++-------------
 block/qcow2.c        |  6 ++----
 3 files changed, 13 insertions(+), 18 deletions(-)

Comments

Alberto Garcia Sept. 17, 2020, 4:35 p.m. UTC | #1
On Wed 09 Sep 2020 08:59:26 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> -/* qcow2_load_dirty_bitmaps()
> - * Return value is a hint for caller: true means that the Qcow2 header was
> - * updated. (false doesn't mean that the header should be updated by the
> - * caller, it just means that updating was not needed or the image cannot be
> - * written to).
> - * On failure the function returns false.
> - */
> -bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> +/* Return true on success, false on failure. */
> +bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
> +                              Error **errp)

I think that the documentation should clarify under what conditions
'header_updated' is modified.

>      if (s->nb_bitmaps == 0) {
>          /* No bitmaps - nothing to do */
> -        return false;
> +        return true;
>      }

Here is it not for example (should it be set to false?).

> -    if (bm_list == NULL) {
> +    if (!bm_list) {
>          return false;
>      }

This looks like a cosmetic change unrelated to the rest of the patch.

Berto
Vladimir Sementsov-Ogievskiy Sept. 17, 2020, 6:58 p.m. UTC | #2
17.09.2020 19:35, Alberto Garcia wrote:
> On Wed 09 Sep 2020 08:59:26 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

>> -/* qcow2_load_dirty_bitmaps()

>> - * Return value is a hint for caller: true means that the Qcow2 header was

>> - * updated. (false doesn't mean that the header should be updated by the

>> - * caller, it just means that updating was not needed or the image cannot be

>> - * written to).

>> - * On failure the function returns false.

>> - */

>> -bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)

>> +/* Return true on success, false on failure. */

>> +bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,

>> +                              Error **errp)

> 

> I think that the documentation should clarify under what conditions

> 'header_updated' is modified.

> 

>>       if (s->nb_bitmaps == 0) {

>>           /* No bitmaps - nothing to do */

>> -        return false;

>> +        return true;

>>       }

> 

> Here is it not for example (should it be set to false?).


Ha, I think, it just shows that patch is wrong :) We should set header_updated at least on every success path. Or better always (if it is non-NULL of course). Thanks for careful review!

> 

>> -    if (bm_list == NULL) {

>> +    if (!bm_list) {

>>           return false;

>>       }

> 

> This looks like a cosmetic change unrelated to the rest of the patch.

> 

> Berto

> 



-- 
Best regards,
Vladimir
diff mbox series

Patch

diff --git a/block/qcow2.h b/block/qcow2.h
index ac6a2d3e2a..e7e662533b 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -966,7 +966,8 @@  void qcow2_cache_discard(Qcow2Cache *c, void *table);
 int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
                                   void **refcount_table,
                                   int64_t *refcount_table_size);
-bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
+                              Error **errp);
 bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
                                 Qcow2BitmapInfoList **info_list, Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 9b14c0791f..f58923fce3 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -959,30 +959,24 @@  static void set_readonly_helper(gpointer bitmap, gpointer value)
     bdrv_dirty_bitmap_set_readonly(bitmap, (bool)value);
 }
 
-/* qcow2_load_dirty_bitmaps()
- * Return value is a hint for caller: true means that the Qcow2 header was
- * updated. (false doesn't mean that the header should be updated by the
- * caller, it just means that updating was not needed or the image cannot be
- * written to).
- * On failure the function returns false.
- */
-bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+/* Return true on success, false on failure. */
+bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
+                              Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     Qcow2BitmapList *bm_list;
     Qcow2Bitmap *bm;
     GSList *created_dirty_bitmaps = NULL;
-    bool header_updated = false;
     bool needs_update = false;
 
     if (s->nb_bitmaps == 0) {
         /* No bitmaps - nothing to do */
-        return false;
+        return true;
     }
 
     bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
                                s->bitmap_directory_size, errp);
-    if (bm_list == NULL) {
+    if (!bm_list) {
         return false;
     }
 
@@ -1033,7 +1027,9 @@  bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
             error_setg_errno(errp, -ret, "Can't update bitmap directory");
             goto fail;
         }
-        header_updated = true;
+        if (header_updated) {
+            *header_updated = true;
+        }
     }
 
     if (!can_write(bs)) {
@@ -1044,7 +1040,7 @@  bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
     g_slist_free(created_dirty_bitmaps);
     bitmap_list_free(bm_list);
 
-    return header_updated;
+    return true;
 
 fail:
     g_slist_foreach(created_dirty_bitmaps, release_dirty_bitmap_helper, bs);
diff --git a/block/qcow2.c b/block/qcow2.c
index eb7c82120c..c2cd9434cc 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1297,7 +1297,6 @@  static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
     unsigned int len, i;
     int ret = 0;
     QCowHeader header;
-    Error *local_err = NULL;
     uint64_t ext_end;
     uint64_t l1_vm_state_index;
     bool update_header = false;
@@ -1786,9 +1785,8 @@  static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
 
     if (!(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) {
         /* It's case 1, 2 or 3.2. Or 3.1 which is BUG in management layer. */
-        bool header_updated = qcow2_load_dirty_bitmaps(bs, &local_err);
-        if (local_err != NULL) {
-            error_propagate(errp, local_err);
+        bool header_updated;
+        if (!qcow2_load_dirty_bitmaps(bs, &header_updated, errp)) {
             ret = -EINVAL;
             goto fail;
         }