QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: John Snow <jsnow@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	qemu-block@nongnu.org
Cc: fam@euphon.net, kwolf@redhat.com, qemu-devel@nongnu.org,
	armbru@redhat.com, mreitz@redhat.com, den@openvz.org
Subject: Re: [Qemu-devel] [PATCH v2 2/3] block/dirty-bitmap: return int from bdrv_remove_persistent_dirty_bitmap
Date: Fri, 13 Sep 2019 18:01:22 -0400
Message-ID: <717dad2d-3a78-e64d-6155-a062cd2b0df1@redhat.com> (raw)
In-Reply-To: <20190911150054.90936-3-vsementsov@virtuozzo.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;
>          }
>      }
> 


  reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-11 15:00 [Qemu-devel] [PATCH v2 0/3] proper locking on bitmap add/remove paths Vladimir Sementsov-Ogievskiy
2019-09-11 15:00 ` [Qemu-devel] [PATCH v2 1/3] block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c Vladimir Sementsov-Ogievskiy
2019-09-13 21:51   ` John Snow
2019-09-11 15:00 ` [Qemu-devel] [PATCH v2 2/3] block/dirty-bitmap: return int from bdrv_remove_persistent_dirty_bitmap Vladimir Sementsov-Ogievskiy
2019-09-13 22:01   ` John Snow [this message]
2019-09-16  8:22     ` Vladimir Sementsov-Ogievskiy
2019-09-11 15:00 ` [Qemu-devel] [PATCH v2 3/3] block/qcow2: proper locking on bitmap add/remove paths Vladimir Sementsov-Ogievskiy
2019-09-13 22:26   ` John Snow

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=717dad2d-3a78-e64d-6155-a062cd2b0df1@redhat.com \
    --to=jsnow@redhat.com \
    --cc=armbru@redhat.com \
    --cc=den@openvz.org \
    --cc=fam@euphon.net \
    --cc=kwolf@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=vsementsov@virtuozzo.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org qemu-devel@archiver.kernel.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox