|
| From: | Max Reitz |
| Subject: | Re: [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:
[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
| [Prev in Thread] | Current Thread | [Next in Thread] |