Re: [Qemu-devel] [PATCH v3 1/5] block/dirty-bitmaps: add user_modifiable status checker

[John Snow]

On 09/26/2018 10:17 PM, Eric Blake wrote:
> On 9/26/18 6:53 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 26.09.2018 02:49, John Snow wrote:
>>> Instead of both frozen and qmp_locked checks, wrap it into one check.
>>> frozen implies the bitmap is split in two (for backup), and shouldn't
>>> be modified. qmp_locked implies it's being used by another operation,
>>> like being exported over NBD. In both cases it means we shouldn't allow
>>> the user to modify it in any meaningful way.
>>>
>>> Replace any usages where we check both frozen and qmp_locked with the
>>> new check.
>>>
>>> Signed-off-by: John Snow <address@hidden>
>>> ---
>>>   block/dirty-bitmap.c         |  6 ++++++
>>>   blockdev.c                   | 29 ++++++++---------------------
>>>   include/block/dirty-bitmap.h |  1 +
>>>   3 files changed, 15 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>> index 8ac933cf1c..fc10543ab0 100644
>>> --- a/block/dirty-bitmap.c
>>> +++ b/block/dirty-bitmap.c
>>> @@ -176,6 +176,12 @@ bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap
>>> *bitmap)
>>>       return bitmap->successor;
>>>   }
>>> +/* Both conditions disallow user-modification via QMP. */
>>> +bool bdrv_dirty_bitmap_user_modifiable(BdrvDirtyBitmap *bitmap) {
>>> +    return !(bdrv_dirty_bitmap_frozen(bitmap) ||
>>> +             bdrv_dirty_bitmap_qmp_locked(bitmap));
>>> +}
>>
>> to reduce number of '!', we may use opposite check, for ex
>> "bdrv_dirty_bitmap_user_locked".
> 
> Meaning make this function return true if locked for one less negation
> in the function body...
> 
>>
>> anyway,
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>
> 
>>> +++ b/blockdev.c
>>> @@ -2009,11 +2009,8 @@ static void
>>> block_dirty_bitmap_clear_prepare(BlkActionState *common,
>>>           return;
>>>       }
>>> -    if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
>>> -        error_setg(errp, "Cannot modify a frozen bitmap");
>>> -        return;
>>> -    } else if (bdrv_dirty_bitmap_qmp_locked(state->bitmap)) {
>>> -        error_setg(errp, "Cannot modify a locked bitmap");
>>> +    if (!bdrv_dirty_bitmap_user_modifiable(state->bitmap)) {
>>> +        error_setg(errp, "Cannot modify a bitmap in-use by another
>>> operation");
>>>           return;
> 
> ...and since most callers were negating sense as well?
> 
> I'm not sure I'm a fan of "in-use" with the hyphen. It sounds better to
> me to just spell it out as two words. (multiple instances)
> 

I'll change both; since you both remarked on the double negation. (Just
how my brain thinks, I suppose -- I prefer routines that check the
positive instead of the negation, but most callers do indeed want the
negation.)