Re: [PATCH] qcow2-bitmaps: fix qcow2_can_store_new_dirty_bitmap

[John Snow]

On 10/10/19 11:39 AM, Eric Blake wrote:
> On 6/7/19 1:53 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 07.06.2019 21:48, Vladimir Sementsov-Ogievskiy wrote:
>>> qcow2_can_store_new_dirty_bitmap works wrong, as it considers only
>>> bitmaps already stored in the qcow2 image and ignores persistent
>>> BdrvDirtyBitmap objects.
>>>
>>> So, let's instead count persistent BdrvDirtyBitmaps. We load all qcow2
>>> bitmaps on open, so there should not be any bitmap in the image for
>>> which we don't have BdrvDirtyBitmaps version. If it is - it's a kind of
>>> corruption, and no reason to check for corruptions here (open() and
>>> close() are better places for it).
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>> ---
>>>
>>> Hi!
>>>
>>> Patch is better than discussing I thing, so here is my
>>> counter-suggestion for
>>> "[PATCH 0/5] block/dirty-bitmap: check number and size constraints
>>> against queued bitmaps"
>>> by John.
>>>
>>> It's of course needs some additional refactoring, as first assert
>>> shows bad design,
>>> I just wrote it in such manner to make minimum changes to fix the bug.
>>>
> 
>>> +    assert(!bdrv_find_dirty_bitmap(bs, name));
>>
>> exactly bad, this should be checked in qmp_block_dirty_bitmap_add(),
>> before checks around
>> persistence. and aio_context_acquire may be dropped..
>>
>> But anyway, idea is clear I think, consider it as RFC patch
> 
> Are you planning to post a v2, as this affects at least
> https://bugzilla.redhat.com/show_bug.cgi?id=1712636
> 

I suppose we ought to do it this way for now as it's less SLOC than my
idea, but I have to admit I would still prefer to get rid of "can_store"
altogether and provide concrete bitmap_store() and bitmap_remove()
callbacks for purpose of symmetry and model simplicity.

I guess I'll worry about that discussion some other time.

--js