Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps

[Greg Kurz]

s/startus/status

On Wed,  9 Sep 2020 21:59:27 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> It's better to return status together with setting errp. It makes
> possible to avoid error propagation.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  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 e7e662533b..49824be5c6 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -972,7 +972,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 f58923fce3..5eeff1cb1c 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1524,9 +1524,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();

Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes
an error_prepend(errp, ...) not visible in the patch context.

Anyway,

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

>      BdrvDirtyBitmap *bitmap;
>      BDRVQcow2State *s = bs->opaque;
>      uint32_t new_nb_bitmaps = s->nb_bitmaps;
> @@ -1546,7 +1547,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;
>          }
>      }
>  
> @@ -1661,7 +1662,7 @@ success:
>      }
>  
>      bitmap_list_free(bm_list);
> -    return;
> +    return true;
>  
>  fail:
>      QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> @@ -1679,16 +1680,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;
>      }
>