Re: [PATCH v2 2/6] blockdev: Split off basic bitmap operations for qemu-img

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3152 bytes --]

On 21.04.20 23:20, Eric Blake wrote:
> Upcoming patches want to add some basic bitmap manipulation abilities
> to qemu-img.  But blockdev.o is too heavyweight to link into qemu-img
> (among other things, it would drag in block jobs and transaction
> support - qemu-img does offline manipulation, where atomicity is less
> important because there are no concurrent modifications to compete
> with), so it's time to split off the bare bones of what we will need
> into a new file blockbitmaps.o.
> 
> In addition to exposing 6 QMP commands for use by qemu-img (add,
> remove, clear, enable, disable, merge), this also has to export three
> previously-static functions for use by blockdev.c transactions.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  Makefile.objs             |   2 +-
>  include/sysemu/blockdev.h |  14 ++
>  blockbitmaps.c            | 324 ++++++++++++++++++++++++++++++++++++++

Hm.  Can we get a better name?  blockdev-bitmaps.c, for example?

>  blockdev.c                | 293 ----------------------------------
>  MAINTAINERS               |   1 +
>  5 files changed, 340 insertions(+), 294 deletions(-)
>  create mode 100644 blockbitmaps.c

[...]

> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index a86d99b3d875..523b7493b1cd 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -57,4 +57,18 @@ QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
>  DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type,
>                       Error **errp);
> 
> +BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
> +                                           const char *name,
> +                                           BlockDriverState **pbs,
> +                                           Error **errp);
> +BdrvDirtyBitmap *do_block_dirty_bitmap_remove(const char *node,
> +                                              const char *name, bool release,
> +                                              BlockDriverState **bitmap_bs,
> +                                              Error **errp);
> +BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node,
> +                                             const char *target,
> +                                             BlockDirtyBitmapMergeSourceList *bitmaps,
> +                                             HBitmap **backup, Error **errp);

Putting do_* functions into a normal header seems a bit weird.  Would
these fit better into block_int.h?

> +
>  #endif

[...]
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8cbc1fac2bfc..769cd357d281 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1989,6 +1989,7 @@ T: git https://github.com/jnsnow/qemu.git jobs
>  Block QAPI, monitor, command line
>  M: Markus Armbruster <armbru@redhat.com>
>  S: Supported
> +F: blockbitmaps.c

The natural choice for something split off of blockdev.c, but I wonder
if the Dirty Bitmaps section wouldn’t be a better fit.

Max

>  F: blockdev.c
>  F: blockdev-hmp-cmds.c
>  F: block/qapi.c
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]