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

[Greg Kurz]

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