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