Re: [PATCH] qcow2-bitmaps: fix qcow2_can_store_new_dirty_bitmap

[John Snow]

On 10/11/19 6:02 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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?
> 

Not sure. I'm going to send the PR out today and you can decide what's
best to do. (Please do CC qemu-stable, though.)

--js