Re: [PATCH] qcow2-bitmaps: fix qcow2_can_store_new_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

10.10.2019 21:46, John Snow wrote:
> 
> 
> 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
> 

Should I base it on master or on you bitmaps branch? Do we want it for stable?

-- 
Best regards,
Vladimir