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 <js...@redhat.com>
> ---
> 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
