Re: [Qemu-devel] [PATCH v3 07/10] block/dirty-bitmaps: unify qmp_locked and user_locked calls

[John Snow]

On 2/25/19 7:03 AM, Vladimir Sementsov-Ogievskiy wrote:
> 23.02.2019 3:06, 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>
> 
> Hmm, and here, you directly fixed what I've noticed, so my r-b,
> of course, still applies.
> 

Sorry for not adding them. I didn't plan to merge the series until you
got a chance to review it, so I wasn't worried about missing it in the
long run. Sorry if it makes it harder to see which ones you'd like to
look at.

> Ha, but I noticed funny thing:
> 
>> ---
>>   block/dirty-bitmap.c           | 40 +++++++++++++++-------------------
>>   blockdev.c                     | 18 +++++++--------
>>   include/block/dirty-bitmap.h   |  5 ++---
>>   migration/block-dirty-bitmap.c |  6 ++---
>>   nbd/server.c                   |  6 ++---
>>   5 files changed, 34 insertions(+), 41 deletions(-)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index d92a269753..b3627b0d8c 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
> 
> [..]
> 
>> @@ -265,9 +259,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState 
>> *bs,
>>       child->disabled = bitmap->disabled;
>>       bitmap->disabled = true;
>>   
>> -    /* Install the successor and lock the parent */
>> +    /* Install the successor and mark the parent as busy */
> 
> I asked you to fix comment for bdrv_dirty_bitmap_create_successor(), but I 
> didn't
> noticed this one, I meant the comment to the whole function, placed above it, 
> which
> now have
>   "Requires that the bitmap is not user_locked and has no successor."
> 
> So, it should be updated too.
> 
> and with it:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> 

Ha. OK, updated to "is not marked busy"

Thanks for the reviews!

>>       bitmap->successor = child;
>> -    bitmap->qmp_locked = true;
>> +    bitmap->busy = true;
>>       return 0;
>>   }
>>   
>