Re: [PATCH v3 1/1] qemu-img: add sub-command --remove-all to 'qemu-img bitmap'

[Kevin Wolf <kwolf@redhat.com>]

Am 24.04.2026 um 12:59 hat Denis V. Lunev geschrieben:
> From time to time it is needed to remove all bitmaps from the image.
> Before this patch the process is not very convenient. One should
> perform
>     qemu-img info
> and parse the output to obtain all names. After that one should
> sequentially call
>     qemu-img bitmap --remove
> for each present bitmap.
> 
> The patch adds --remove-all sub-command to 'qemu-img bitmap'.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Hanna Reitz <hreitz@redhat.com>
> ---
> Changes from v2:
> * rebased to the lastest head
> 
> Changes from v1:
> * rebased to latest head
> * adopted bitmap help to the new layout
> 
>  docs/tools/qemu-img.rst | 10 +++++---
>  qemu-img.c              | 53 ++++++++++++++++++++++++++++++++++-------
>  2 files changed, 52 insertions(+), 11 deletions(-)
> 
> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index 558b0eb84d..b0c798b77a 100644
> --- a/docs/tools/qemu-img.rst
> +++ b/docs/tools/qemu-img.rst
> @@ -301,15 +301,19 @@ Command description:
>    For write tests, by default a buffer filled with zeros is written. This can be
>    overridden with a pattern byte specified by *PATTERN*.
>  
> -.. option:: bitmap (--merge SOURCE | --add | --remove | --clear | --enable | --disable)... [-b SOURCE_FILE [-F SOURCE_FMT]] [-g GRANULARITY] [--object OBJECTDEF] [--image-opts | -f FMT] FILENAME BITMAP
> +.. option:: bitmap (--merge SOURCE | --add | --remove | --remove-all | --clear | --enable | --disable)... [-b SOURCE_FILE [-F SOURCE_FMT]] [-g GRANULARITY] [--object OBJECTDEF] [--image-opts | -f FMT] FILENAME [BITMAP]
>  
> -  Perform one or more modifications of the persistent bitmap *BITMAP*
> -  in the disk image *FILENAME*.  The various modifications are:
> +  Perform one or more modifications of persistent bitmaps in the disk
> +  image *FILENAME*.  Most operations require *BITMAP* to be specified;
> +  ``--remove-all`` operates on all bitmaps and does not take *BITMAP*.
> +  The various modifications are:
>  
>    ``--add`` to create *BITMAP*, enabled to record future edits.
>  
>    ``--remove`` to remove *BITMAP*.
>  
> +  ``--remove-all`` to remove all bitmaps.
> +
>    ``--clear`` to clear *BITMAP*.
>  
>    ``--enable`` to change *BITMAP* to start recording future edits.
> diff --git a/qemu-img.c b/qemu-img.c
> index c42dd4e995..22ddc7a627 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -87,6 +87,7 @@ enum {
>      OPTION_FORCE = 276,
>      OPTION_SKIP_BROKEN = 277,
>      OPTION_LIMITS = 278,
> +    OPTION_REMOVE_ALL = 279,
>  };
>  
>  typedef enum OutputFormat {
> @@ -5018,6 +5019,7 @@ enum ImgBitmapAct {
>      BITMAP_ENABLE,
>      BITMAP_DISABLE,
>      BITMAP_MERGE,
> +    BITMAP_REMOVE_ALL,
>  };
>  typedef struct ImgBitmapAction {
>      enum ImgBitmapAct act;
> @@ -5036,7 +5038,7 @@ static int img_bitmap(const img_cmd_t *ccmd, int argc, char **argv)
>      BlockDriverState *bs = NULL, *src_bs = NULL;
>      bool image_opts = false;
>      int64_t granularity = 0;
> -    bool add = false, merge = false;
> +    bool add = false, merge = false, remove_all = false, any = false;

It's quite unclear what "any" is supposed to mean. From the code it
looks like it's set when a bitmap name must be specified (which is the
opposite of what I'd think of with "any").

Can we use something more descriptive like need_bitmap_name?

>      QSIMPLEQ_HEAD(, ImgBitmapAction) actions;
>      ImgBitmapAction *act, *act_next;
>      const char *op;
> @@ -5052,6 +5054,7 @@ static int img_bitmap(const img_cmd_t *ccmd, int argc, char **argv)
>              {"add", no_argument, 0, OPTION_ADD},
>              {"granularity", required_argument, 0, 'g'},
>              {"remove", no_argument, 0, OPTION_REMOVE},
> +            {"remove-all", no_argument, 0, OPTION_REMOVE_ALL},
>              {"clear", no_argument, 0, OPTION_CLEAR},
>              {"enable", no_argument, 0, OPTION_ENABLE},
>              {"disable", no_argument, 0, OPTION_DISABLE},
> @@ -5070,8 +5073,8 @@ static int img_bitmap(const img_cmd_t *ccmd, int argc, char **argv)
>          switch (c) {
>          case 'h':
>              cmd_help(ccmd, "[-f FMT | --image-opts]\n"
> -"        ( --add [-g SIZE] | --remove | --clear | --enable | --disable |\n"
> -"          --merge SOURCE [-b SRC_FILE [-F SRC_FMT]] )..\n"
> +"        ( --add [-g SIZE] | --remove | --remove-all | --clear | --enable |\n"
> +"          --disable | --merge SOURCE [-b SRC_FILE [-F SRC_FMT]] )..\n"
>  "        [--object OBJDEF] FILE BITMAP\n"
>  ,
>  "  -f, --format FMT\n"
> @@ -5086,6 +5089,8 @@ static int img_bitmap(const img_cmd_t *ccmd, int argc, char **argv)
>  "     with optional multiplier suffix (in powers of 1024)\n"
>  "  --remove\n"
>  "     removes BITMAP from FILE\n"
> +"  --remove-all\n"
> +"     removes all bitmaps from FILE\n"
>  "  --clear\n"
>  "     clears BITMAP in FILE\n"
>  "  --enable, --disable\n"
> @@ -5115,7 +5120,7 @@ static int img_bitmap(const img_cmd_t *ccmd, int argc, char **argv)
>              act = g_new0(ImgBitmapAction, 1);
>              act->act = BITMAP_ADD;
>              QSIMPLEQ_INSERT_TAIL(&actions, act, next);
> -            add = true;
> +            add = any = true;

One assignment per line, please.

>              break;
>          case 'g':
>              granularity = cvtnum("granularity", optarg, true);
> @@ -5127,28 +5132,38 @@ static int img_bitmap(const img_cmd_t *ccmd, int argc, char **argv)
>              act = g_new0(ImgBitmapAction, 1);
>              act->act = BITMAP_REMOVE;
>              QSIMPLEQ_INSERT_TAIL(&actions, act, next);
> +            any = true;
> +            break;
> +        case OPTION_REMOVE_ALL:
> +            act = g_new0(ImgBitmapAction, 1);
> +            act->act = BITMAP_REMOVE_ALL;
> +            QSIMPLEQ_INSERT_TAIL(&actions, act, next);
> +            remove_all = true;
>              break;
>          case OPTION_CLEAR:
>              act = g_new0(ImgBitmapAction, 1);
>              act->act = BITMAP_CLEAR;
>              QSIMPLEQ_INSERT_TAIL(&actions, act, next);
> +            any = true;
>              break;
>          case OPTION_ENABLE:
>              act = g_new0(ImgBitmapAction, 1);
>              act->act = BITMAP_ENABLE;
>              QSIMPLEQ_INSERT_TAIL(&actions, act, next);
> +            any = true;
>              break;
>          case OPTION_DISABLE:
>              act = g_new0(ImgBitmapAction, 1);
>              act->act = BITMAP_DISABLE;
>              QSIMPLEQ_INSERT_TAIL(&actions, act, next);
> +            any = true;
>              break;
>          case OPTION_MERGE:
>              act = g_new0(ImgBitmapAction, 1);
>              act->act = BITMAP_MERGE;
>              act->src = optarg;
>              QSIMPLEQ_INSERT_TAIL(&actions, act, next);
> -            merge = true;
> +            any = merge = true;

Same here.

>              break;
>          case 'b':
>              src_filename = optarg;
> @@ -5165,8 +5180,8 @@ static int img_bitmap(const img_cmd_t *ccmd, int argc, char **argv)
>      }
>  
>      if (QSIMPLEQ_EMPTY(&actions)) {
> -        error_report("Need at least one of --add, --remove, --clear, "
> -                     "--enable, --disable, or --merge");
> +        error_report("Need at least one of --add, --remove, --remove-all, "
> +                     "--clear, --enable, --disable, or --merge");
>          goto out;
>      }
>  
> @@ -5184,10 +5199,14 @@ static int img_bitmap(const img_cmd_t *ccmd, int argc, char **argv)
>          goto out;
>      }
>  
> -    if (optind != argc - 2) {
> +    if (any && optind != argc - 2) {
>          error_report("Expecting filename and bitmap name");
>          goto out;
>      }
> +    if (!any && remove_all && optind != argc - 1) {

Doesn't !any already imply remove_all? If both weren't set, actions
would be empty and we'd already have errored out above.

This probably means that remove_all can go away entirely because it's
only read in this condition.

> +        error_report("Expecting filename");
> +        goto out;
> +    }
>  
>      filename = argv[optind];
>      bitmap = argv[optind + 1];
> @@ -5225,6 +5244,24 @@ static int img_bitmap(const img_cmd_t *ccmd, int argc, char **argv)
>              qmp_block_dirty_bitmap_remove(bs->node_name, bitmap, &err);
>              op = "remove";
>              break;
> +        case BITMAP_REMOVE_ALL: {
> +            while (1) {
> +                const char *name;
> +                BdrvDirtyBitmap *bm = bdrv_dirty_bitmap_first(bs);
> +                if (bm == NULL) {
> +                    break;
> +                }

while ((bm = bdrv_dirty_bitmap_first(bs)))?

> +                name = bdrv_dirty_bitmap_name(bm);
> +                qmp_block_dirty_bitmap_remove(bs->node_name, name, &err);
> +                if (err) {
> +                    /* Save name for proper error reporting */
> +                    bitmap = name;
> +                    break;
> +                }
> +            }
> +            op = "remove-all";
> +            break;
> +        }
>          case BITMAP_CLEAR:
>              qmp_block_dirty_bitmap_clear(bs->node_name, bitmap, &err);
>              op = "clear";

Kevin