On 2/19/19 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:
> 19.02.2019 1:32, John Snow wrote:
>>
>>
>> On 2/18/19 8:57 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 14.02.2019 2:23, John Snow wrote:
>>>> "Frozen" was a good description a long time ago, but it isn't adequate now.
>>>> Rename the frozen predicate to has_successor to make the semantics of the
>>>> predicate more clear to outside callers.
>>>>
>>>> In the process, remove some calls to frozen() that no longer semantically
>>>> make sense. For enabled and disabled in particular, it's actually okay for
>>>> the internals to do this but only forbidden for users to invoke them, and
>>>
>>> I'm a bit lost in this paragraph.. to this - what?, to invoke them - whom?
>>> I think, it would be simpler for me to read patch itself :)
>>>
>>
>> Touched this up. I meant enable and disable, not enabled and disabled.
>>
>>>> all of the QMP entry uses already check against qmp_locked.
>>>>
>>>> Several other assertions really want to check that the bitmap isn't in-use
>>>> by another operation -- use the qmp_locked function for this instead, which
>>>> presently also checks for has_successor.
>>>
>>> hm, you mean user_locked, not qmp_locked.
>>>
>>
>> Yes.
>>
>> [...]
>>
>>>> /**
>>>> * Create a successor bitmap destined to replace this bitmap after an operation.
>>>> - * Requires that the bitmap is not frozen and has no successor.
>>>> + * Requires that the bitmap is not locked and has no successor.
>>>
>>> I think, user_locked, to not interfere with bitmaps mutex. And you use user_locked in
>>> other comments in this patch.
>>>
>>
>> You're right. It gets changed again later, but I didn't make this easy
>> to read.
>>
>>>> * Called with BQL taken.
>>>> */
>>>> int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
>>>> @@ -244,12 +244,16 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
>>>> uint64_t granularity;
>>>> BdrvDirtyBitmap *child;
>>>>
>>>> - if (bdrv_dirty_bitmap_frozen(bitmap)) {
>>>> - error_setg(errp, "Cannot create a successor for a bitmap that is "
>>>> - "currently frozen");
>>>> + if (bdrv_dirty_bitmap_user_locked(bitmap)) {
>>>> + error_setg(errp, "Cannot create a successor for a bitmap that is in-use "
>>>> + "by an operation");
>>>> + return -1;
>>>> + }
>>>> + if (bdrv_dirty_bitmap_has_successor(bitmap)) {
>>>> + error_setg(errp, "Cannot create a successor for a bitmap that already "
>>>> + "has one");
>>>
>>>
>>> Amm, dead code? _user_locked() implies no successor, so we instead can keep an assertion..
>>>
>>
>> It gets changed later in the series, but I didn't do a great job of
>> explaining that in advance. I'll amend the commit message to explain
>> what I'm trying to do.
>>
>> I tried to hint at this with: "which presently also checks for
>> has_successor" as an admission that it was redundant, but I need to call
>> it out in stronger language.
>>
>
> Hmm, isn't it better to keep an assertion, than add dead code, to be fixed in later commits?
>
Eh. I wrote code that looked semantically correct without worrying about
what the calls actually do:
- We want to make sure this bitmap is not in use (user_locked,
qmp_locked, or busy -- however you want to spell it), and
- We want to make sure this bitmap doesn't have a successor.
Now, you and I happen to know that these two conditions aren't actually
independent, but that's not necessarily obvious from this one function
to a new reader. Adding the second check gives some assurance to the reader.
In my mind, the concept of having a successor is now distinct from that
of being busy, and create_successor actually wants to check both things:
(1) That is able to create a successor, because it doesn't have one, and
(2) That it is not modifying a bitmap in use by some operation.
But, you're right, there's no way to have a bitmap with a successor that
isn't busy, so an assertion will suffice for the instructional purpose.
I'll change it.
--js
