[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Max Reitz
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH 05/11] block: add delayed bitmap successor cleanup
Date: Wed, 18 Mar 2015 09:03:41 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0

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:


+    }
+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.


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
+    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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]