On 5/15/26 15:55, Kevin Wolf wrote:
> 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 <[email protected]>
>> CC: Kevin Wolf <[email protected]>
>> CC: Hanna Reitz <[email protected]>
>> ---
>> 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?
correct. I have spent at least 5 minutes trying to guess
what I mean under this name. Meaning was "any single bitmap
operation has been added to the list".
Actually non-obvious naming long term. This is good catch.
need_bitmap_name is nice.
>> 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.
Correct. We have had a check that at least one action is specified
if (QSIMPLEQ_EMPTY(&actions)) {
error_report("Need at least one of --add, --remove, --remove-all, "
"--clear, --enable, --disable, or --merge");
goto out;
}
and thus !any de-facto means that --remove-all is set and there
are no other actions added.
>> + 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
The rest is clear.
Thank you for the review,
Den
