Re: [Qemu-devel] [PATCH v2 09/18] block/dirty-bitmap: add bdrv_dirty_bitmap_merge_internal

[Max Reitz]

On 03.07.19 23:55, John Snow wrote:
> I'm surprised it didn't come up sooner, but sometimes we have a +busy
> bitmap as a source. This is dangerous from the QMP API, but if we are
> the owner that marked the bitmap busy, it's safe to merge it using it as
> a read only source.
> 
> It is not safe in the general case to allow users to read from in-use
> bitmaps, so create an internal variant that foregoes the safety
> checking.
> 
> Signed-off-by: John Snow <address@hidden>
> ---
>  block/dirty-bitmap.c      | 51 +++++++++++++++++++++++++++++++++------
>  include/block/block_int.h |  3 +++
>  2 files changed, 47 insertions(+), 7 deletions(-)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 95a9c2a5d8..b0f76826b3 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -810,11 +810,15 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap 
> *bitmap,
>      return hbitmap_next_dirty_area(bitmap->bitmap, offset, bytes);
>  }
>  
> +/**
> + * bdrv_merge_dirty_bitmap: merge src into dest.
> + * Ensures permissions on bitmaps are reasonable; use for public API.
> + *
> + * @backup: If provided, make a copy of dest here prior to merge.
> + */
>  void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap 
> *src,
>                               HBitmap **backup, Error **errp)
>  {
> -    bool ret;
> -
>      qemu_mutex_lock(dest->mutex);
>      if (src->mutex != dest->mutex) {
>          qemu_mutex_lock(src->mutex);
> @@ -833,6 +837,37 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, 
> const BdrvDirtyBitmap *src,
>          goto out;
>      }
>  
> +    assert(bdrv_dirty_bitmap_merge_internal(dest, src, backup, false));

Please keep the explicit @ret.  We never define NDEBUG, but doing things
with side effects inside of assert() is bad style nonetheless.

> +
> +out:
> +    qemu_mutex_unlock(dest->mutex);
> +    if (src->mutex != dest->mutex) {
> +        qemu_mutex_unlock(src->mutex);
> +    }
> +}
> +
> +/**
> + * bdrv_dirty_bitmap_merge_internal: merge src into dest.
> + * Does NOT check bitmap permissions; not suitable for use as public API.
> + *
> + * @backup: If provided, make a copy of dest here prior to merge.
> + * @lock: If true, lock and unlock bitmaps on the way in/out.
> + * returns true if the merge succeeded; false if unattempted.
> + */
> +bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
> +                                      const BdrvDirtyBitmap *src,
> +                                      HBitmap **backup,
> +                                      bool lock)
> +{
> +    bool ret;
> +
> +    if (lock) {
> +        qemu_mutex_lock(dest->mutex);
> +        if (src->mutex != dest->mutex) {
> +            qemu_mutex_lock(src->mutex);
> +        }
> +    }
> +

Why not check for INCONSISTENT and RO still?

Max

>      if (backup) {
>          *backup = dest->bitmap;
>          dest->bitmap = hbitmap_alloc(dest->size, 
> hbitmap_granularity(*backup));

signature.asc
Description: OpenPGP digital signature