On 7/4/19 12:49 PM, Max Reitz wrote:
> 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.
>
Thank you for saving me from myself. I consistently forget this :(
>> +
>> +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
>
Well. "why", I guess. The user-facing API will always use the
non-internal version. This is the scary dangerous version that you don't
call unless you are Very Sure You Know What You're Doing.
I guess I just intended for the suitability checking to happen in
bdrv_dirty_bitmap_merge, and this is the implementation that you can
shoot yourself in the foot with if you'd like.
>> if (backup) {
>> *backup = dest->bitmap;
>> dest->bitmap = hbitmap_alloc(dest->size,
>> hbitmap_granularity(*backup));
>
