Re: [Qemu-devel] [PATCH v2 2/3] block/dirty-bitmap: return int from bdrv_remove_persistent_dirty_bitmap

[John Snow <jsnow@redhat.com>]

On 9/11/19 11:00 AM, Vladimir Sementsov-Ogievskiy wrote:
> It's more comfortable to not deal with local_err.
> 

I agree.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2.h                |  5 ++---
>  include/block/block_int.h    |  6 +++---
>  include/block/dirty-bitmap.h |  5 ++---
>  block/dirty-bitmap.c         |  9 +++++----
>  block/qcow2-bitmap.c         | 20 +++++++++++---------
>  blockdev.c                   |  7 +++----
>  6 files changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 998bcdaef1..99ee88f802 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -747,9 +747,8 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>                                        const char *name,
>                                        uint32_t granularity,
>                                        Error **errp);
> -void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> -                                          const char *name,
> -                                          Error **errp);
> +int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
> +                                         Error **errp);
>  
>  ssize_t coroutine_fn
>  qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 0422acdf1c..503ac9e3cd 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -556,9 +556,9 @@ struct BlockDriver {
>                                              const char *name,
>                                              uint32_t granularity,
>                                              Error **errp);
> -    void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
> -                                                const char *name,
> -                                                Error **errp);
> +    int (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
> +                                               const char *name,
> +                                               Error **errp);
>  
>      /**
>       * Register/unregister a buffer for I/O. For example, when the driver is
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 4b4b731b46..07503b03b5 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -37,9 +37,8 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
>                              Error **errp);
>  void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
>  void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
> -void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> -                                         const char *name,
> -                                         Error **errp);
> +int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
> +                                        Error **errp);
>  void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
>  void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
>  void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap);
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 8f42015db9..a52b83b619 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -455,13 +455,14 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
>   * not fail.
>   * This function doesn't release corresponding BdrvDirtyBitmap.
>   */
> -void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> -                                         const char *name,
> -                                         Error **errp)
> +int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
> +                                        Error **errp)
>  {
>      if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) {
> -        bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
> +        return bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
>      }
> +

But is it a problem if we return an error code without setting errp now?
If this is for the sake of not having to deal with local_err, we should
make sure that a non-zero return means that errp is set. Right?

> +    return -ENOTSUP;
>  }
>  
>  bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index b2487101ed..1aaedb3b55 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1404,11 +1404,10 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list,
>      return NULL;
>  }
>  
> -void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> -                                          const char *name,
> -                                          Error **errp)
> +int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
> +                                         Error **errp)
>  {
> -    int ret;
> +    int ret = 0;

I was going to say I'd rather not initialize this, but is this related
to ubsan linting?

>      BDRVQcow2State *s = bs->opaque;
>      Qcow2Bitmap *bm;
>      Qcow2BitmapList *bm_list;
> @@ -1416,18 +1415,19 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>      if (s->nb_bitmaps == 0) {
>          /* Absence of the bitmap is not an error: see explanation above
>           * bdrv_remove_persistent_dirty_bitmap() definition. */
> -        return;
> +        return 0;
>      }
>  
>      bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>                                 s->bitmap_directory_size, errp);
>      if (bm_list == NULL) {
> -        return;
> +        return -EIO;
>      }
>  
>      bm = find_bitmap_by_name(bm_list, name);
>      if (bm == NULL) {
> -        goto fail;
> +        ret = -EINVAL;
> +        goto out;
>      }
>  
>      QSIMPLEQ_REMOVE(bm_list, bm, Qcow2Bitmap, entry);
> @@ -1435,14 +1435,16 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>      ret = update_ext_header_and_dir(bs, bm_list);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret, "Failed to update bitmap extension");
> -        goto fail;
> +        goto out;
>      }
>  
>      free_bitmap_clusters(bs, &bm->table);
>  
> -fail:
> +out:
>      bitmap_free(bm);
>      bitmap_list_free(bm_list);
> +
> +    return ret;
>  }
>  
>  void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> diff --git a/blockdev.c b/blockdev.c
> index fbef6845c8..0813adfb2b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2940,15 +2940,14 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
>      }
>  
>      if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
> +        int ret;
>          AioContext *aio_context = bdrv_get_aio_context(bs);
> -        Error *local_err = NULL;
>  
>          aio_context_acquire(aio_context);
> -        bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
> +        ret = bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
>          aio_context_release(aio_context);
>  
> -        if (local_err != NULL) {
> -            error_propagate(errp, local_err);
> +        if (ret < 0) {
>              return NULL;
>          }
>      }
>