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 <vsement...@virtuozzo.com> >>>> --- >>>> >>>> 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
