Re: [Qemu-devel] [PATCH 07/29] qcow2-bitmap: add qcow2_bitmap_load()

[Kevin Wolf <kwolf@redhat.com>]

Am 08.08.2016 um 17:04 hat Vladimir Sementsov-Ogievskiy geschrieben:
> This function loads block dirty bitmap from qcow2.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

As you said this is really unused at the moment, I'll mostly skip this
patch. Just one thing:

> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 1fe0fd9..2c11ad7 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -224,6 +224,10 @@ struct BlockDriver {
>      int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
>      ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs);
>  
> +    BdrvDirtyBitmap *(*bdrv_dirty_bitmap_load)(BlockDriverState *bs,
> +                                               const char *name,
> +                                               Error **errp);
> +

I would prefer adding BlockDriver callbacks in the same patch as the
wrapper function, and keeping them separate from the first format driver
implementation. The reason for this is that sometimes people want to
backport the infrastructure, but not the driver change.

Similar to my comment on an earlier series, this means doing patch 16
(which this hunk added) first and only then implementing the function in
qcow2.

This specific instance probably disappears anyway as you remove the dead
code for now, but just to clarify what the preferred order is generally
(and I think you're going to re-submit it later, right?)

Kevin