diff mbox series

[v2,09/13] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface

Message ID 20200917195519.19589-10-vsementsov@virtuozzo.com
State New
Headers show
Series block: deal with errp: part I | expand

Commit Message

Vladimir Sementsov-Ogievskiy Sept. 17, 2020, 7:55 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 | 25 ++++++++++++++-----------
 block/qcow2.c        |  6 ++----
 3 files changed, 18 insertions(+), 16 deletions(-)

Comments

Alberto Garcia Sept. 18, 2020, 2:54 p.m. UTC | #1 
On Thu 17 Sep 2020 09:55:15 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> 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>


> +/*

> + * Return true on success, false on failure. Anyway, if header_updated

> + * provided set it appropriately.

>   */


I'm not a native speaker but it sounds a bit odd to me. Maybe "If
header_updated is not NULL then it is set appropriately regardless of
the return value".

But I'm fine with your version, so

Reviewed-by: Alberto Garcia <berto@igalia.com>


Berto
Vladimir Sementsov-Ogievskiy Sept. 18, 2020, 2:59 p.m. UTC | #2 
18.09.2020 17:54, Alberto Garcia wrote:
> On Thu 17 Sep 2020 09:55:15 PM CEST, Vladimir Sementsov-Ogievskiy wrote:

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

> 

>> +/*

>> + * Return true on success, false on failure. Anyway, if header_updated

>> + * provided set it appropriately.

>>    */

> 

> I'm not a native speaker but it sounds a bit odd to me. Maybe "If

> header_updated is not NULL then it is set appropriately regardless of

> the return value".


That's better I think, thanks.

> 

> But I'm fine with your version, so

> 

> Reviewed-by: Alberto Garcia <berto@igalia.com>

> 

> Berto

> 



-- 
Best regards,
Vladimir
Greg Kurz Sept. 18, 2020, 3:15 p.m. UTC | #3 
On Thu, 17 Sep 2020 22:55:15 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

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

> ---


With the documentation change suggested by Berto,

Reviewed-by: Greg Kurz <groug@kaod.org>


>  block/qcow2.h        |  3 ++-

>  block/qcow2-bitmap.c | 25 ++++++++++++++-----------

>  block/qcow2.c        |  6 ++----

>  3 files changed, 18 insertions(+), 16 deletions(-)

> 

> diff --git a/block/qcow2.h b/block/qcow2.h

> index 6eac088f1c..3c64dcda33 100644

> --- a/block/qcow2.h

> +++ b/block/qcow2.h

> @@ -972,7 +972,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 4f6138f544..500175f4e8 100644

> --- a/block/qcow2-bitmap.c

> +++ b/block/qcow2-bitmap.c

> @@ -962,25 +962,26 @@ 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.

> +/*

> + * Return true on success, false on failure. Anyway, if header_updated

> + * provided set it appropriately.

>   */

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

> +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 (header_updated) {

> +        *header_updated = 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,

> @@ -1036,7 +1037,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)) {

> @@ -1047,7 +1050,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 8c89c98978..c4b86df7c0 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;

> @@ -1785,9 +1784,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;

>          }
diff mbox series

Patch

diff --git a/block/qcow2.h b/block/qcow2.h
index 6eac088f1c..3c64dcda33 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -972,7 +972,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 4f6138f544..500175f4e8 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -962,25 +962,26 @@  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.
+/*
+ * Return true on success, false on failure. Anyway, if header_updated
+ * provided set it appropriately.
  */
-bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+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 (header_updated) {
+        *header_updated = 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,
@@ -1036,7 +1037,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)) {
@@ -1047,7 +1050,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 8c89c98978..c4b86df7c0 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;
@@ -1785,9 +1784,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;
         }