Re: [Qemu-devel] [PATCH 05/11] block: add delayed bitmap successor cleanup

[Max Reitz <mreitz@redhat.com>]

On 2015-03-17 at 18:46, John Snow wrote:
>
>
> On 03/17/2015 02:44 PM, Max Reitz wrote:
>> On 2015-03-04 at 23:15, John Snow wrote:

[snip]

>>> +    }
>>> +}
>>> +
>>> +BdrvDirtyBitmap *bdrv_dirty_bitmap_decref(BlockDriverState *bs,
>>
>> I don't know whether I'm that content with the name chosen, because
>> you're actually decrementing the refcount of the successor; but since
>> the successor is basically a clone of the original bitmap (and I mean in
>> the Star Trek sense, that it's a teleported clone and the original is
>> intended to be destroyed so the successor can replace it), decrementing
>> the refcount of the successor basically is equal to decrementing the
>> refcount of the bitmap itself (as long as there is a successor, which
>> you are asserting; maybe you want to add a comment about that to
>> include/block/block.h, that one can only use this on frozen bitmaps?).
>>
>
> I could get clever with the name and call it 
> bdrv_frozen_bitmap_decref, which hopefully still shows membership to 
> the bdrv_dirty_bitmap class of functions but clarifies its usage 
> sufficiently.

Sounds good to me.

[snip]

>>> diff --git a/block/backup.c b/block/backup.c
>>> index 41bd9af..4332df4 100644
>>> --- a/block/backup.c
>>> +++ b/block/backup.c
>>> @@ -240,6 +240,12 @@ static void backup_complete(BlockJob *job, void
>>> *opaque)
>>>       bdrv_unref(s->target);
>>> +    if (s->sync_bitmap) {
>>> +        BdrvDirtyBitmap *bm;
>>> +        bm = bdrv_dirty_bitmap_decref(job->bs, s->sync_bitmap,
>>> data->ret, NULL);
>>> +        assert(bm);
>>
>> You can use &error_abort as the Error object and drop the assert(); or,
>> if you are dropping the Error parameter, there is no need to check the
>> return value at all, because it will always be non-NULL (there won't be
>> any code path in the function returning NULL at all). Maybe you can even
>> drop the return value, too.
>>
>> I just looked through the series: Actually, you're never using the Error
>> parameter for bdrv_dirty_bitmap_decref() at all. Seems to me like you
>> really should drop it (and maybe the return value along with it).
>>
>
> I actually use this parameter to return the "new bitmap" (if any) 
> after the decrement operation. I wanted to leave the window open for 
> merge optimizations, so I tell the caller which bitmap remains after 
> the operation.
>
> I will cull the errp, but will likely leave the return code.

OK.

Max