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 <[email protected]> >>> --- >>> 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
