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

From: John Snow <jsnow@redhat.com>
To: Max Reitz <mreitz@redhat.com>,
	qemu-devel@nongnu.org, qemu-block@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>, Fam Zheng <fam@euphon.net>,
	vsementsov@virtuozzo.com, Wen Congyang <wencongyang2@huawei.com>,
	Xie Changlong <xiechanglong.d@gmail.com>,
	Markus Armbruster <armbru@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 06/12] block/dirty-bitmap: add bdrv_dirty_bitmap_claim
Date: Thu, 20 Jun 2019 12:36:53 -0400	[thread overview]
Message-ID: <c956b213-24bf-355a-02e3-6b39ba682ae4@redhat.com> (raw)
In-Reply-To: <f9296483-d48f-521f-1ddd-d514e56b8fdf@redhat.com>

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.

> 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 <jsnow@redhat.com>
>> ---
>>  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
> 