On 13.07.2016 09:57, Vladimir Sementsov-Ogievskiy wrote: > On 13.07.2016 01:49, John Snow wrote: >> >> On 06/03/2016 12:32 AM, Fam Zheng wrote: >>> HBitmap is an implementation detail of block dirty bitmap that should >>> be hidden >>> from users. Introduce a BdrvDirtyBitmapIter to encapsulate the >>> underlying >>> HBitmapIter. >>> >>> A small difference in the interface is, before, an HBitmapIter is >>> initialized >>> in place, now the new BdrvDirtyBitmapIter must be dynamically >>> allocated because >>> the structure definition is in block/dirty-bitmap.c. >>> >>> Two current users are converted too. >>> >>> Signed-off-by: Fam Zheng <f...@redhat.com> >>> --- >>> block/backup.c | 14 ++++++++------ >>> block/dirty-bitmap.c | 39 >>> +++++++++++++++++++++++++++++++++------ >>> block/mirror.c | 24 +++++++++++++----------- >>> include/block/dirty-bitmap.h | 7 +++++-- >>> include/qemu/typedefs.h | 1 + >>> 5 files changed, 60 insertions(+), 25 deletions(-) >>>
[...] >>> @@ -224,6 +231,7 @@ static void >>> bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs, >>> BdrvDirtyBitmap *bm, *next; >>> QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) { >>> if ((!bitmap || bm == bitmap) && (!only_named || bm->name)) { >>> + assert(!bitmap->active_iterators); >> No good -- this function is also used to clear out all named bitmaps >> belonging to a BDS, so 'bitmap' may be NULL, so this assertion is bad. > > Just a note about this. I do not like this hidden behaviour of > release_bitmap, as it more native for freeing functions to do nothing > with NULL pointer.. g_free(NULL) do not free all allocations))).. If > someone agrees, this may be better to be changed. The function is not called "bdrv_release_dirty_bitmap()", though, but "bdrv_do_release_matching_dirty_bitmap()". The "do" means that it's an internal function, called only by bdrv_release_dirty_bitmap() (aha!) and bdrv_release_named_dirty_bitmaps(); and the "matching" means that if you pass NULL, it'll release all bitmaps. What we could do is make bdrv_release_dirty_bitmap() a no-op if NULL is passed, or add an assertion that the argument is not NULL. I'd be fine with either, but I don't think bdrv_do_release_matching_dirty_bitmap() needs to be adjusted. Max
signature.asc
Description: OpenPGP digital signature