Re: [Qemu-devel] [PATCH v5 06/10] block: add delayed bitmap successor cleanup

[John Snow]

On 06/05/2015 08:56 AM, Stefan Hajnoczi wrote:
> On Thu, Jun 04, 2015 at 05:46:08PM -0400, John Snow wrote:
>> @@ -3190,19 +3193,13 @@ BdrvDirtyBitmap
>> *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, * we may wish
>> to re-join the parent and child/successor. * The merged parent
>> will be un-frozen, but not explicitly re-enabled. */ 
>> -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState
>> *bs, -                                           BdrvDirtyBitmap
>> *parent, -                                           Error
>> **errp) +static BdrvDirtyBitmap
>> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, +
>> BdrvDirtyBitmap *parent) { BdrvDirtyBitmap *successor =
>> parent->successor;
>> 
>> -    if (!successor) { -        error_setg(errp, "Cannot reclaim
>> a successor when none is present"); -        return NULL; -    } 
>> - +    assert(successor); if (!hbitmap_merge(parent->bitmap,
>> successor->bitmap)) { -        error_setg(errp, "Merging of
>> parent and successor bitmap failed"); return NULL; }
> 
> Is this reachable?  If the bitmap size and granularity match then 
> hbitmap_merge() does not fail.
> 
> This should probably be covered with an assertion instead to show
> that this is not allowed to happen.
> 

You're right, I was thinking "What if something changes under my feet?
This layer does not know about the HBitmap internals," but for now
they do match and an assertion is appropriate to guard against
accidental changes.

--js