Re: [Qemu-block] [PATCH v2 5/6] block/dirty-bitmaps: unify qmp_locked and user_locked calls

[John Snow]

On 2/18/19 12:27 PM, Vladimir Sementsov-Ogievskiy wrote:
> 14.02.2019 2:23, John Snow wrote:
>> These mean the same thing now. Unify them and rename the merged call
>> bdrv_dirty_bitmap_busy to indicate semantically what we are describing,
>> as well as help disambiguate from the various _locked and _unlocked
>> versions of bitmap helpers that refer to mutex locks.
>>
>> Signed-off-by: John Snow <address@hidden>
>> Reviewed-by: Eric Blake <address@hidden>
>> ---
>>   block/dirty-bitmap.c           | 41 +++++++++++++++-------------------
>>   blockdev.c                     | 18 +++++++--------
>>   include/block/dirty-bitmap.h   |  5 ++---
>>   migration/block-dirty-bitmap.c |  6 ++---
>>   nbd/server.c                   |  6 ++---
>>   5 files changed, 35 insertions(+), 41 deletions(-)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 2042c62602..8ab048385a 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -48,9 +48,9 @@ struct BdrvDirtyBitmap {
>>       QemuMutex *mutex;
>>       HBitmap *bitmap;            /* Dirty bitmap implementation */
>>       HBitmap *meta;              /* Meta dirty bitmap */
>> -    bool qmp_locked;            /* Bitmap is locked, it can't be modified
>> -                                   through QMP */
>> -    BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked 
>> state */
>> +    bool busy;                  /* Bitmap is busy, it can't be modified 
>> through
>> +                                   QMP */
> 
> better not "modified" but "used".. for example, export through NBD is not a 
> modification.
> 

True.

>> +    BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
> 
> hm this comment change about successor relates more to previous patch, but I 
> don't really care.
> 

Oh, true.

>>       char *name;                 /* Optional non-empty unique ID */
>>       int64_t size;               /* Size of the bitmap, in bytes */
>>       bool disabled;              /* Bitmap is disabled. It ignores all 
>> writes to
>> @@ -188,22 +188,17 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap 
>> *bitmap)
>>       return bitmap->successor;
>>   }
>>   
> 
> 
> In comment for bdrv_dirty_bitmap_create_successor, there is "locked" word, 
> which you forget to fix to "busy"
> with at least this fixed:

Good spot. Too many occurrences of the word "lock" to have looked for
them all.

> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> 
>