Re: [PATCH 09/14] block/qcow2: qcow2_get_specific_info(): drop error propagation

[Greg Kurz]

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

> Don't use error propagation in qcow2_get_specific_info(). For this
> refactor qcow2_get_bitmap_info_list, its current interface rather

... interface is rather

> weird.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

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

>  block/qcow2.h        |  4 ++--
>  block/qcow2-bitmap.c | 27 +++++++++++++--------------
>  block/qcow2.c        | 10 +++-------
>  3 files changed, 18 insertions(+), 23 deletions(-)
> 
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 065ec3df0b..ac6a2d3e2a 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -967,8 +967,8 @@ 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);
> -Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
> -                                                Error **errp);
> +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,
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 8c34b2aef7..9b14c0791f 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1090,30 +1090,29 @@ static Qcow2BitmapInfoFlagsList 
> *get_bitmap_info_flags(uint32_t flags)
>  /*
>   * qcow2_get_bitmap_info_list()
>   * Returns a list of QCOW2 bitmap details.
> - * In case of no bitmaps, the function returns NULL and
> - * the @errp parameter is not set.
> - * When bitmap information can not be obtained, the function returns
> - * NULL and the @errp parameter is set.
> + * On success return true with bm_list set (probably to NULL, if no bitmaps),
> + * on failure return false with errp set.
>   */
> -Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
> -                                                Error **errp)
> +bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
> +                                Qcow2BitmapInfoList **info_list, Error 
> **errp)
>  {
>      BDRVQcow2State *s = bs->opaque;
>      Qcow2BitmapList *bm_list;
>      Qcow2Bitmap *bm;
> -    Qcow2BitmapInfoList *list = NULL;
> -    Qcow2BitmapInfoList **plist = &list;
>  
>      if (s->nb_bitmaps == 0) {
> -        return NULL;
> +        *info_list = NULL;
> +        return true;
>      }
>  
>      bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>                                 s->bitmap_directory_size, errp);
> -    if (bm_list == NULL) {
> -        return NULL;
> +    if (!bm_list) {
> +        return false;
>      }
>  
> +    *info_list = NULL;
> +
>      QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>          Qcow2BitmapInfo *info = g_new0(Qcow2BitmapInfo, 1);
>          Qcow2BitmapInfoList *obj = g_new0(Qcow2BitmapInfoList, 1);
> @@ -1121,13 +1120,13 @@ Qcow2BitmapInfoList 
> *qcow2_get_bitmap_info_list(BlockDriverState *bs,
>          info->name = g_strdup(bm->name);
>          info->flags = get_bitmap_info_flags(bm->flags & ~BME_RESERVED_FLAGS);
>          obj->value = info;
> -        *plist = obj;
> -        plist = &obj->next;
> +        *info_list = obj;
> +        info_list = &obj->next;
>      }
>  
>      bitmap_list_free(bm_list);
>  
> -    return list;
> +    return true;
>  }
>  
>  int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 10175fa399..eb7c82120c 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -5056,12 +5056,10 @@ static ImageInfoSpecific 
> *qcow2_get_specific_info(BlockDriverState *bs,
>      BDRVQcow2State *s = bs->opaque;
>      ImageInfoSpecific *spec_info;
>      QCryptoBlockInfo *encrypt_info = NULL;
> -    Error *local_err = NULL;
>  
>      if (s->crypto != NULL) {
> -        encrypt_info = qcrypto_block_get_info(s->crypto, &local_err);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> +        encrypt_info = qcrypto_block_get_info(s->crypto, errp);
> +        if (!encrypt_info) {
>              return NULL;
>          }
>      }
> @@ -5078,9 +5076,7 @@ static ImageInfoSpecific 
> *qcow2_get_specific_info(BlockDriverState *bs,
>          };
>      } else if (s->qcow_version == 3) {
>          Qcow2BitmapInfoList *bitmaps;
> -        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> +        if (!qcow2_get_bitmap_info_list(bs, &bitmaps, errp)) {
>              qapi_free_ImageInfoSpecific(spec_info);
>              qapi_free_QCryptoBlockInfo(encrypt_info);
>              return NULL;