Re: [Qemu-block] [Qemu-devel] [PATCH v5 01/10] block: Hide HBitmap in block dirty bitmap interface

[Max Reitz]

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 <address@hidden>
>>> ---
>>>   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