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

[Max Reitz]

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.

I see a weak reason to add some form of a check (and be it just an
assertion), and that is that if someone needs to remove that check,
they’ll have to explicitly justify why that is OK.

Just like this patch justifies why it’s sometimes OK to call this
function on a busy bitmap.


So I feel weakly.

> 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.

Some still contain assertions, though.

Max

signature.asc
Description: OpenPGP digital signature