[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 06/12] block/dirty-bitmap: add bdrv_dirty_bitmap
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [Qemu-block] [PATCH 06/12] block/dirty-bitmap: add bdrv_dirty_bitmap_claim |
Date: |
Fri, 21 Jun 2019 11:58:48 +0000 |
20.06.2019 19:36, John Snow wrote:
>
>
> On 6/20/19 12:02 PM, Max Reitz wrote:
>> On 20.06.19 03:03, John Snow wrote:
>>> This function can claim an hbitmap to replace its own current hbitmap.
>>> In the case that the granularities do not match, it will use
>>> hbitmap_merge to copy the bit data instead.
>>
>> I really do not like this name because to me it implies a relationship
>> to bdrv_reclaim_dirty_bitmap(). Maybe just bdrv_dirty_bitmap_take()?
>> Or, if you want to get more fancy, bdrv_dirty_dirty_bitmap_steal().
>> (Because references are taken or stolen.)
>>
>
> take or steal is good. I just wanted to highlight that it truly takes
> ownership. The double-pointer and erasing the caller's reference for
> emphasis of this idea.
Didn't you consider bdrv_dirty_bitmap_set_hbitmap? Hmm, but your function
actually eats pointer, so 'set' is not enough.. bdrv_dirty_bitmap_eat_hbitmap?
And to be serious: it is the point where we definitely should drop HBitmap:meta
field, as it in bad relation with parent hbitmap stealing.
>
>> The latter might fit in nicely with the abdication theme. We’d just
>> need to rename bdrv_reclaim_dirty_bitmap() to
>> bdrv_dirty_bitmap_backstab(), and it’d be perfect.
>>
>
> Don't tempt me; I do like my silly function names. You are lucky I don't
> call
>
>> (On another note: bdrv_restore_dirty_bitmap() vs.
>> bdrv_dirty_bitmap_restore() – really? :-/)
>>
>
> I have done a terrible job at enforcing any kind of consistency here,
> and it gets me all the time too. I had a big series that re-arranged and
> re-named a ton of stuff just to make things a little more nicer, but I
> let it bitrot because I didn't want to deal with the bike-shedding.
>
> I do agree I am in desperate need of a spring cleaning in here.
>
> One thing that does upset me quite often is that the canonical name for
> the structure is "bdrv dirty bitmap", which makes the function names all
> quite long.
>
>>> Signed-off-by: John Snow <address@hidden>
>>> ---
>>> include/block/block_int.h | 1 +
>>> include/qemu/hbitmap.h | 8 ++++++++
>>> block/dirty-bitmap.c | 14 ++++++++++++++
>>> util/hbitmap.c | 5 +++++
>>> 4 files changed, 28 insertions(+)
>>
>> The implementation looks good to me.
>>
>> Max
>>
>
--
Best regards,
Vladimir
- Re: [Qemu-block] [PATCH 07/12] block/backup: add 'always' bitmap sync policy, (continued)
- Re: [Qemu-block] [PATCH 07/12] block/backup: add 'always' bitmap sync policy, Vladimir Sementsov-Ogievskiy, 2019/06/21
- Re: [Qemu-block] [PATCH 07/12] block/backup: add 'always' bitmap sync policy, Vladimir Sementsov-Ogievskiy, 2019/06/21
- Re: [Qemu-block] [Qemu-devel] [PATCH 07/12] block/backup: add 'always' bitmap sync policy, John Snow, 2019/06/21
- Re: [Qemu-block] [Qemu-devel] [PATCH 07/12] block/backup: add 'always' bitmap sync policy, Max Reitz, 2019/06/21
- Re: [Qemu-block] [Qemu-devel] [PATCH 07/12] block/backup: add 'always' bitmap sync policy, John Snow, 2019/06/21
[Qemu-block] [PATCH 10/12] iotests: teach FilePath to produce multiple paths, John Snow, 2019/06/19
[Qemu-block] [PATCH 06/12] block/dirty-bitmap: add bdrv_dirty_bitmap_claim, John Snow, 2019/06/19
[Qemu-block] [PATCH 04/12] hbitmap: Fix merge when b is empty, and result is not an alias of a, John Snow, 2019/06/19
[Qemu-block] [PATCH 01/12] qapi: add BitmapSyncMode enum, John Snow, 2019/06/19
[Qemu-block] [PATCH 02/12] block/backup: Add mirror sync mode 'bitmap', John Snow, 2019/06/19