Re: [Qemu-devel] [PATCH v2 09/18] block/dirty-bitmap: add bdrv_dirty_bitmap_merge_internal

[John Snow]

On 7/8/19 7:44 AM, Max Reitz wrote:
> On 05.07.19 18:45, John Snow wrote:
>>
>>
>> On 7/4/19 12:49 PM, Max Reitz wrote:
>>> On 03.07.19 23:55, John Snow wrote:
> 
> [...]
> 
>>>> +
>>>> +/**
>>>> + * bdrv_dirty_bitmap_merge_internal: merge src into dest.
>>>> + * Does NOT check bitmap permissions; not suitable for use as public API.
>>>> + *
>>>> + * @backup: If provided, make a copy of dest here prior to merge.
>>>> + * @lock: If true, lock and unlock bitmaps on the way in/out.
>>>> + * returns true if the merge succeeded; false if unattempted.
>>>> + */
>>>> +bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
>>>> +                                      const BdrvDirtyBitmap *src,
>>>> +                                      HBitmap **backup,
>>>> +                                      bool lock)
>>>> +{
>>>> +    bool ret;
>>>> +
>>>> +    if (lock) {
>>>> +        qemu_mutex_lock(dest->mutex);
>>>> +        if (src->mutex != dest->mutex) {
>>>> +            qemu_mutex_lock(src->mutex);
>>>> +        }
>>>> +    }
>>>> +
>>>
>>> Why not check for INCONSISTENT and RO still?
>>>
>>> Max
>>>
>>
>> Well. "why", I guess. The user-facing API will always use the
>> non-internal version. This is the scary dangerous version that you don't
>> call unless you are Very Sure You Know What You're Doing.
>>
>> I guess I just intended for the suitability checking to happen in
>> bdrv_dirty_bitmap_merge, and this is the implementation that you can
>> shoot yourself in the foot with if you'd like.
> 
> I’m asking because the reasoning behind being allowed to call this
> function is that BUSY means someone who is not the monitor has exclusive
> access to the bitmap, and that someone is the caller of this function.
> 

Unfortunately, there's no way mechanically to check that this is the
exact circumstance the function is being called in. I have named it
_internal and documented the source to explain when it's safe to use.

We do not keep track of who set +BUSY and therefore who is allowed to
call functions that normally prohibit that flag from being set.

I don't think it's worth implementing that, either.

> There is no justification for why it should be allowed to call this
> function for bitmaps that are inconsistent or read-only.  If someone
> needs that, they should justify it with a patch, I think.

The only caller here has already verified that this bitmap is not
inconsistent or read-only (because the caller MADE the bitmap). Do you
feel strongly enough that the check should be duplicated for this one
particular function?

There are many other dirty bitmap functions that, because they are
called as an interior function not directly invoked by the QMP API
layer, do not do any such checking.

GIGO, surely?

--js