Re: [Qemu-block] [PATCH 06/12] block/dirty-bitmap: add bdrv_dirty_bitmap_claim

[Vladimir Sementsov-Ogievskiy]

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