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 <js...@redhat.com>
>>> ---
>>> 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 <vsement...@virtuozzo.com>
>>
>
>>> +++ 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.)