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

[John Snow]

On 7/8/19 2:33 PM, Max Reitz wrote:
> On 08.07.19 20:24, John Snow wrote:
>>
>>
>> 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.
> 
> Fully agreed.  I meant to say that calling this function on a busy
> bitmap is completely fine, so I understand why there is no check and I
> wouldn’t add one.
> 
>>> 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).
> 
> Which is why implementing it would be trivial, as the caller could just
> pass &error_abort.
> 
> Well, or the function just asserts that !readonly && !inconsistent.
> (Which would probably be better, because bdrv_dirty_bitmap_check() is
> probably something better suited for external interfaces.  No need to
> use it if all callers of bdrv_dirty_bitmap_merge_internal() only pass
> &error_abort anyway.)
> 
>> Do you
>> feel strongly enough that the check should be duplicated for this one
>> particular function?
> 
> I don’t see a good reason not to.
> 

"Fine," but I'll want the V3 changes eyed over first before I post a fixup.