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

[John Snow]

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 <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.
> 

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));
>