[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 01/10] block: Hide HBitmap in block dirty bit
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v5 01/10] block: Hide HBitmap in block dirty bitmap interface |
Date: |
Wed, 13 Jul 2016 12:10:13 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 |
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