Include actions for --add, --remove, --clear, --enable, --disable, and
--merge (note that --clear is a bit of fluff, because the same can be
accomplished by removing a bitmap and then adding a new one in its
place, but it matches what QMP commands exist). Listing is omitted,
because it does not require a bitmap name and because it was already
possible with 'qemu-img info'. A single command line can play one or
more bitmap commands in sequence on the same bitmap name (although all
added bitmaps share the same granularity, and and all merged bitmaps
come from the same source file). Merge defaults to other bitmaps in
the primary image, but can also be told to merge bitmaps from a
distinct image.
While this supports --image-opts for the file being modified, I did
not think it worth the extra complexity to support that for the source
file in a cross-file merges. Likewise, I chose to have --merge only
take a single source rather than following the QMP support for
multiple merges in one go (although you can still use more than one
--merge in the command line); in part because qemu-img is offline and
therefore atomicity is not an issue.
Upcoming patches will add iotest coverage of these commands while
also testing other features.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
docs/tools/qemu-img.rst | 24 ++++
qemu-img.c | 254 ++++++++++++++++++++++++++++++++++++++++
qemu-img-cmds.hx | 7 ++
3 files changed, 285 insertions(+)
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 7d08c48d308f..219483cec279 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -281,6 +281,30 @@ 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
+
+ Perform one or more modifications of the persistent bitmap *BITMAP*
+ in the disk image *FILENAME*. The various modifications are:
+
+ ``--add`` to create *BITMAP*, enabled to record future edits.
+
+ ``--remove`` to remove *BITMAP*.
+
+ ``--clear`` to clear *BITMAP*.
+
+ ``--enable`` to change *BITMAP* to start recording future edits.
+
+ ``--disable`` to change *BITMAP* to stop recording future edits.
+
+ ``--merge`` to merge the contents of *SOURCE_BITMAP* into *BITMAP*.
+
+ Additional options include ``-g`` which sets a non-default
+ *GRANULARITY* for ``--add``, and ``-b`` and ``-F`` which select an
+ alternative source file for all *SOURCE* bitmaps used by
+ ``--merge``.
+
+ To see what bitmaps are present in an image, use ``qemu-img info``.
+
.. option:: check [--object OBJECTDEF] [--image-opts] [-q] [-f FMT] [--output=OFMT] [-r [leaks | all]] [-T SRC_CACHE] [-U] FILENAME
Perform a consistency check on the disk image *FILENAME*. The command can
diff --git a/qemu-img.c b/qemu-img.c
index b6e8af9202a5..8c99e68ba8aa 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -28,6 +28,7 @@
#include "qemu-common.h"
#include "qemu-version.h"
#include "qapi/error.h"
+#include "qapi/qapi-commands-block-core.h"
#include "qapi/qapi-visit-block-core.h"
#include "qapi/qobject-output-visitor.h"
#include "qapi/qmp/qjson.h"
@@ -71,6 +72,12 @@ enum {
OPTION_SHRINK = 266,
OPTION_SALVAGE = 267,
OPTION_TARGET_IS_ZERO = 268,
+ OPTION_ADD = 269,
+ OPTION_REMOVE = 270,
+ OPTION_CLEAR = 271,
+ OPTION_ENABLE = 272,
+ OPTION_DISABLE = 273,
+ OPTION_MERGE = 274,
};
typedef enum OutputFormat {
@@ -169,6 +176,14 @@ static void QEMU_NORETURN help(void)
" '-n' skips the target volume creation (useful if the volume is created\n"
" prior to running qemu-img)\n"
"\n"
+ "Parameters to bitmap subcommand:\n"
+ " 'bitmap' is the name of the bitmap to manipulate, through one or more\n"
+ " actions from '--add', '--remove', '--clear', '--enable', '--disable',\n"
+ " or '--merge source'\n"
+ " '-g granularity' sets the granularity for '--add' actions\n"
+ " '-b source' and '-F src_fmt' tell '--merge' actions to find the source\n"
+ " bitmaps from an alternative file\n"
+ "\n"
"Parameters to check subcommand:\n"
" '-r' tries to repair any inconsistencies that are found during the check.\n"
" '-r leaks' repairs only cluster leaks, whereas '-r all' fixes all\n"
@@ -4461,6 +4476,245 @@ out:
return 0;
}
+enum ImgBitmapAct {
+ BITMAP_ADD,
+ BITMAP_REMOVE,
+ BITMAP_CLEAR,
+ BITMAP_ENABLE,
+ BITMAP_DISABLE,
+ BITMAP_MERGE,
+};
+typedef struct ImgBitmapAction {
+ enum ImgBitmapAct act;
+ const char *src; /* only used for merge */
+ QSIMPLEQ_ENTRY(ImgBitmapAction) next;
+} ImgBitmapAction;
+
+static int img_bitmap(int argc, char **argv)
+{
+ Error *err = NULL;
+ int c, ret = -1;
+ QemuOpts *opts = NULL;
+ const char *fmt = NULL, *src_fmt = NULL, *src_filename = NULL;
+ const char *filename, *bitmap;
+ BlockBackend *blk = NULL, *src = NULL;
+ BlockDriverState *bs = NULL, *src_bs = NULL;
+ bool image_opts = false;
+ int64_t granularity = 0;
+ bool add = false, merge = false;
+ QSIMPLEQ_HEAD(, ImgBitmapAction) actions;
+ ImgBitmapAction *act, *act_next;
+ const char *op;
+
+ QSIMPLEQ_INIT(&actions);
+
+ for (;;) {
+ static const struct option long_options[] = {
+ {"help", no_argument, 0, 'h'},
+ {"object", required_argument, 0, OPTION_OBJECT},
+ {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+ {"add", no_argument, 0, OPTION_ADD},
+ {"remove", no_argument, 0, OPTION_REMOVE},
+ {"clear", no_argument, 0, OPTION_CLEAR},
+ {"enable", no_argument, 0, OPTION_ENABLE},
+ {"disable", no_argument, 0, OPTION_DISABLE},
+ {"merge", required_argument, 0, OPTION_MERGE},
+ {"granularity", required_argument, 0, 'g'},
+ {"source-file", required_argument, 0, 'b'},
+ {"source-format", required_argument, 0, 'F'},
+ {0, 0, 0, 0}
+ };
+ c = getopt_long(argc, argv, ":b:f:F:g:h", long_options, NULL);
+ if (c == -1) {
+ break;
+ }
+
+ switch (c) {
+ case ':':
+ missing_argument(argv[optind - 1]);
+ break;
+ case '?':
+ unrecognized_option(argv[optind - 1]);
+ break;
+ case 'h':
+ help();
+ break;
+ case 'b':
+ src_filename = optarg;
+ break;
+ case 'f':
+ fmt = optarg;
+ break;
+ case 'F':
+ src_fmt = optarg;
+ break;
+ case 'g':
+ granularity = cvtnum(optarg);
+ if (granularity < 0) {
+ error_report("Invalid granularity specified");
+ return 1;
+ }
+ break;
+ case OPTION_ADD:
+ act = g_new0(ImgBitmapAction, 1);
+ act->act = BITMAP_ADD;
+ QSIMPLEQ_INSERT_TAIL(&actions, act, next);
+ add = true;
+ break;
+ case OPTION_REMOVE:
+ act = g_new0(ImgBitmapAction, 1);
+ act->act = BITMAP_REMOVE;
+ QSIMPLEQ_INSERT_TAIL(&actions, act, next);
+ break;
+ case OPTION_CLEAR:
+ act = g_new0(ImgBitmapAction, 1);
+ act->act = BITMAP_CLEAR;
+ QSIMPLEQ_INSERT_TAIL(&actions, act, next);
+ break;
+ case OPTION_ENABLE:
+ act = g_new0(ImgBitmapAction, 1);
+ act->act = BITMAP_ENABLE;
+ QSIMPLEQ_INSERT_TAIL(&actions, act, next);
+ break;
+ case OPTION_DISABLE:
+ act = g_new0(ImgBitmapAction, 1);
+ act->act = BITMAP_DISABLE;
+ QSIMPLEQ_INSERT_TAIL(&actions, act, next);
+ break;
+ case OPTION_MERGE:
+ act = g_new0(ImgBitmapAction, 1);
+ act->act = BITMAP_MERGE;
+ act->src = optarg;
+ QSIMPLEQ_INSERT_TAIL(&actions, act, next);
+ merge = true;
+ break;
+ case OPTION_OBJECT:
+ opts = qemu_opts_parse_noisily(&qemu_object_opts, optarg, true);
+ if (!opts) {
+ goto out;
+ }
+ break;
+ case OPTION_IMAGE_OPTS:
+ image_opts = true;
+ break;
+ }
+ }
+
+ if (qemu_opts_foreach(&qemu_object_opts,
+ user_creatable_add_opts_foreach,
+ qemu_img_object_print_help, &error_fatal)) {
+ goto out;
+ }
+
+ if (QSIMPLEQ_EMPTY(&actions)) {
+ error_report("Need at least one of --add, --remove, --clear, "
+ "--enable, --disable, or --merge");
+ goto out;
+ }
+
+ if (granularity && !add) {
+ error_report("granularity only supported with --add");
+ goto out;
+ }
+ if (src_fmt && !src_filename) {
+ error_report("-F only supported with -b");
+ goto out;
+ }
+ if (src_filename && !merge) {
+ error_report("Merge bitmap source file only supported with "
+ "--merge");
+ goto out;
+ }
+
+ if (optind != argc - 2) {
+ error_report("Expecting filename and bitmap name");
+ goto out;
+ }
+
+ filename = argv[optind];
+ bitmap = argv[optind + 1];
+
+ blk = img_open(image_opts, filename, fmt, BDRV_O_RDWR, false, false,
+ false);
+ if (!blk) {
+ goto out;
+ }
+ bs = blk_bs(blk);
+ if (src_filename) {
+ src = img_open(NULL, src_filename, src_fmt, 0, false, false,
+ false);
+ if (!src) {
+ goto out;
+ }
+ src_bs = blk_bs(src);
+ } else {
+ src_bs = bs;
+ }
+
+ QSIMPLEQ_FOREACH_SAFE(act, &actions, next, act_next) {
+ switch (act->act) {
+ case BITMAP_ADD:
+ qmp_block_dirty_bitmap_add(bs->node_name, bitmap,
+ !!granularity, granularity, true, true,
+ false, false, &err);
+ op = "add";
+ break;
+ case BITMAP_REMOVE:
+ qmp_block_dirty_bitmap_remove(bs->node_name, bitmap, &err);
+ op = "remove";
+ break;
+ case BITMAP_CLEAR:
+ qmp_block_dirty_bitmap_clear(bs->node_name, bitmap, &err);
+ op = "clear";
+ break;
+ case BITMAP_ENABLE:
+ qmp_block_dirty_bitmap_enable(bs->node_name, bitmap, &err);
+ op = "enable";
+ break;
+ case BITMAP_DISABLE:
+ qmp_block_dirty_bitmap_disable(bs->node_name, bitmap, &err);
+ op = "disable";
+ break;
+ case BITMAP_MERGE: {
+ BlockDirtyBitmapMergeSource *merge_src;
+ BlockDirtyBitmapMergeSourceList *list;
+
+ merge_src = g_new0(BlockDirtyBitmapMergeSource, 1);
+ merge_src->type = QTYPE_QDICT;
+ merge_src->u.external.node = g_strdup(src_bs->node_name);
+ merge_src->u.external.name = g_strdup(act->src);
+ list = g_new0(BlockDirtyBitmapMergeSourceList, 1);
+ list->value = merge_src;
+ qmp_block_dirty_bitmap_merge(bs->node_name, bitmap, list, &err);
+ qapi_free_BlockDirtyBitmapMergeSourceList(list);
+ op = "merge";
+ break;
+ }
+ default:
+ g_assert_not_reached();
+ }
+
+ if (err) {
+ error_reportf_err(err, "Operation %s on bitmap %s failed",
+ op, bitmap);
+ ret = -1;
+ goto out;
+ }
+ g_free(act);
+ }
+
+ ret = 0;
+
+ out:
+ blk_unref(src);
+ blk_unref(blk);
+ qemu_opts_del(opts);
+ if (ret) {
+ return 1;
+ }
+ return 0;
+}
+
#define C_BS 01
#define C_COUNT 02
#define C_IF 04
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index e0886437b1f2..011688245668 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -20,6 +20,13 @@ DEF("bench", img_bench,
SRST
.. option:: bench [-c COUNT] [-d DEPTH] [-f FMT] [--flush-interval=FLUSH_INTERVAL] [-i AIO] [-n] [--no-drain] [-o OFFSET] [--pattern=PATTERN] [-q] [-s BUFFER_SIZE] [-S STEP_SIZE] [-t CACHE] [-w] [-U] FILENAME
ERST
+
+DEF("bitmap", img_bitmap,
+ "bitmap (--merge SOURCE | --add | --remove | --clear | --enable | --disable)... [-b source_file [-F source_fmt]] [-g granularity] [--object objectdef] [--image-opts | -f fmt] filename bitmap")
+SRST
+.. 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
+ERST
+
DEF("check", img_check,
"check [--object objectdef] [--image-opts] [-q] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] [-U] filename")
SRST
--
2.26.2
13.05.2020 04:16, Eric Blake wrote:
> Include actions for --add, --remove, --clear, --enable, --disable, and
> --merge (note that --clear is a bit of fluff, because the same can be
> accomplished by removing a bitmap and then adding a new one in its
> place, but it matches what QMP commands exist). Listing is omitted,
> because it does not require a bitmap name and because it was already
> possible with 'qemu-img info'. A single command line can play one or
> more bitmap commands in sequence on the same bitmap name (although all
> added bitmaps share the same granularity, and and all merged bitmaps
> come from the same source file). Merge defaults to other bitmaps in
> the primary image, but can also be told to merge bitmaps from a
> distinct image.
>
> While this supports --image-opts for the file being modified, I did
> not think it worth the extra complexity to support that for the source
> file in a cross-file merges. Likewise, I chose to have --merge only
> take a single source rather than following the QMP support for
> multiple merges in one go (although you can still use more than one
> --merge in the command line); in part because qemu-img is offline and
> therefore atomicity is not an issue.
>
> Upcoming patches will add iotest coverage of these commands while
> also testing other features.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
I'm sorry for asking it only now on v4.. But still. Why do we need it? We can instead run qemu binary (or even new qemu-storage-daemon) and just use existing qmp commands. Is there a real benefit in developing qemu-img, maintaining two interfaces for the same thing? Of-course, just run qmp commands from terminal is a lot less comfortable than just a qemu img command.. But may be we need some wrapper, which make it simple to run one qmp command on an image?
It's simple to make a python wrapper working like
qemu-qmp block-dirty-bitmap-add '{node: self, name: bitmap0, persistent: true}' /path/to/x.qcow2
--
Best regards,
Vladimir
On 5/14/20 1:45 AM, Vladimir Sementsov-Ogievskiy wrote:
> 13.05.2020 04:16, Eric Blake wrote:
>> Include actions for --add, --remove, --clear, --enable, --disable, and
>> --merge (note that --clear is a bit of fluff, because the same can be
>> accomplished by removing a bitmap and then adding a new one in its
>> place, but it matches what QMP commands exist). Listing is omitted,
>> because it does not require a bitmap name and because it was already
>> possible with 'qemu-img info'. A single command line can play one or
>> more bitmap commands in sequence on the same bitmap name (although all
>> added bitmaps share the same granularity, and and all merged bitmaps
>> come from the same source file). Merge defaults to other bitmaps in
>> the primary image, but can also be told to merge bitmaps from a
>> distinct image.
>>
>
> I'm sorry for asking it only now on v4.. But still. Why do we need it?
Ease of use.
> We can instead run qemu binary (or even new qemu-storage-daemon) and
> just use existing qmp commands. Is there a real benefit in developing
> qemu-img, maintaining two interfaces for the same thing?
If it makes someone's life easier, and is not hard to maintain, then
yes. A command line interface that calls into QMP is not hard to
maintain. And _I_ certainly found it easier to write iotests with this
patch in place, so it already has at least one client.
> Of-course, just
> run qmp commands from terminal is a lot less comfortable than just a
> qemu img command.. But may be we need some wrapper, which make it simple
> to run one qmp command on an image?
>
> It's simple to make a python wrapper working like
>
> qemu-qmp block-dirty-bitmap-add '{node: self, name: bitmap0, persistent:
> true}' /path/to/x.qcow2
This _IS_ such a wrapper. The whole point of this patch is that it is
now simpler to run one (or more) QMP command on an offline image from
the command line. Just because I wrote it in C instead of python, and
attached it to an existing tool instead of writing a new tool, doesn't
change the fact that it is just a wrapper around the existing QMP commands.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
14.05.2020 17:20, Eric Blake wrote:
> On 5/14/20 1:45 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 13.05.2020 04:16, Eric Blake wrote:
>>> Include actions for --add, --remove, --clear, --enable, --disable, and
>>> --merge (note that --clear is a bit of fluff, because the same can be
>>> accomplished by removing a bitmap and then adding a new one in its
>>> place, but it matches what QMP commands exist). Listing is omitted,
>>> because it does not require a bitmap name and because it was already
>>> possible with 'qemu-img info'. A single command line can play one or
>>> more bitmap commands in sequence on the same bitmap name (although all
>>> added bitmaps share the same granularity, and and all merged bitmaps
>>> come from the same source file). Merge defaults to other bitmaps in
>>> the primary image, but can also be told to merge bitmaps from a
>>> distinct image.
>>>
>
>>
>> I'm sorry for asking it only now on v4.. But still. Why do we need it?
>
> Ease of use.
>
>> We can instead run qemu binary (or even new qemu-storage-daemon) and just use existing qmp commands. Is there a real benefit in developing qemu-img, maintaining two interfaces for the same thing?
>
> If it makes someone's life easier, and is not hard to maintain, then yes. A command line interface that calls into QMP is not hard to maintain. And _I_ certainly found it easier to write iotests with this patch in place, so it already has at least one client.
>
>> Of-course, just run qmp commands from terminal is a lot less comfortable than just a qemu img command.. But may be we need some wrapper, which make it simple to run one qmp command on an image?
>>
>> It's simple to make a python wrapper working like
>>
>> qemu-qmp block-dirty-bitmap-add '{node: self, name: bitmap0, persistent: true}' /path/to/x.qcow2
>
> This _IS_ such a wrapper. The whole point of this patch is that it is now simpler to run one (or more) QMP command on an offline image from the command line. Just because I wrote it in C instead of python, and attached it to an existing tool instead of writing a new tool, doesn't change the fact that it is just a wrapper around the existing QMP commands.
>
OK, that's right.
The thing that I didn't like is that we have to make cli-to-qapi interface mapping by hand. But I see now that interface you implementing is prepared to make several actions with same bitmap-name, which can't be achieved with some kind of automatic interface matching anyway, so my proposal don't match your needs, sorry for my haste)
--
Best regards,
Vladimir
13.05.2020 04:16, Eric Blake wrote:
> Include actions for --add, --remove, --clear, --enable, --disable, and
> --merge (note that --clear is a bit of fluff, because the same can be
> accomplished by removing a bitmap and then adding a new one in its
> place, but it matches what QMP commands exist). Listing is omitted,
> because it does not require a bitmap name and because it was already
> possible with 'qemu-img info'. A single command line can play one or
> more bitmap commands in sequence on the same bitmap name (although all
> added bitmaps share the same granularity, and and all merged bitmaps
> come from the same source file). Merge defaults to other bitmaps in
> the primary image, but can also be told to merge bitmaps from a
> distinct image.
>
> While this supports --image-opts for the file being modified, I did
> not think it worth the extra complexity to support that for the source
> file in a cross-file merges. Likewise, I chose to have --merge only
> take a single source rather than following the QMP support for
> multiple merges in one go (although you can still use more than one
> --merge in the command line); in part because qemu-img is offline and
> therefore atomicity is not an issue.
>
> Upcoming patches will add iotest coverage of these commands while
> also testing other features.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
> docs/tools/qemu-img.rst | 24 ++++
> qemu-img.c | 254 ++++++++++++++++++++++++++++++++++++++++
> qemu-img-cmds.hx | 7 ++
> 3 files changed, 285 insertions(+)
>
> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index 7d08c48d308f..219483cec279 100644
> --- a/docs/tools/qemu-img.rst
> +++ b/docs/tools/qemu-img.rst
> @@ -281,6 +281,30 @@ 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
> +
> + Perform one or more modifications of the persistent bitmap *BITMAP*
> + in the disk image *FILENAME*. The various modifications are:
> +
> + ``--add`` to create *BITMAP*, enabled to record future edits.
> +
> + ``--remove`` to remove *BITMAP*.
> +
> + ``--clear`` to clear *BITMAP*.
> +
> + ``--enable`` to change *BITMAP* to start recording future edits.
> +
> + ``--disable`` to change *BITMAP* to stop recording future edits.
> +
> + ``--merge`` to merge the contents of *SOURCE_BITMAP* into *BITMAP*.
> +
> + Additional options include ``-g`` which sets a non-default
> + *GRANULARITY* for ``--add``, and ``-b`` and ``-F`` which select an
> + alternative source file for all *SOURCE* bitmaps used by
> + ``--merge``.
> +
> + To see what bitmaps are present in an image, use ``qemu-img info``.
> +
> .. option:: check [--object OBJECTDEF] [--image-opts] [-q] [-f FMT] [--output=OFMT] [-r [leaks | all]] [-T SRC_CACHE] [-U] FILENAME
>
> Perform a consistency check on the disk image *FILENAME*. The command can
> diff --git a/qemu-img.c b/qemu-img.c
> index b6e8af9202a5..8c99e68ba8aa 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -28,6 +28,7 @@
> #include "qemu-common.h"
> #include "qemu-version.h"
> #include "qapi/error.h"
> +#include "qapi/qapi-commands-block-core.h"
> #include "qapi/qapi-visit-block-core.h"
> #include "qapi/qobject-output-visitor.h"
> #include "qapi/qmp/qjson.h"
> @@ -71,6 +72,12 @@ enum {
> OPTION_SHRINK = 266,
> OPTION_SALVAGE = 267,
> OPTION_TARGET_IS_ZERO = 268,
> + OPTION_ADD = 269,
> + OPTION_REMOVE = 270,
> + OPTION_CLEAR = 271,
> + OPTION_ENABLE = 272,
> + OPTION_DISABLE = 273,
> + OPTION_MERGE = 274,
> };
>
> typedef enum OutputFormat {
> @@ -169,6 +176,14 @@ static void QEMU_NORETURN help(void)
> " '-n' skips the target volume creation (useful if the volume is created\n"
> " prior to running qemu-img)\n"
> "\n"
> + "Parameters to bitmap subcommand:\n"
> + " 'bitmap' is the name of the bitmap to manipulate, through one or more\n"
> + " actions from '--add', '--remove', '--clear', '--enable', '--disable',\n"
> + " or '--merge source'\n"
> + " '-g granularity' sets the granularity for '--add' actions\n"
> + " '-b source' and '-F src_fmt' tell '--merge' actions to find the source\n"
> + " bitmaps from an alternative file\n"
> + "\n"
> "Parameters to check subcommand:\n"
> " '-r' tries to repair any inconsistencies that are found during the check.\n"
> " '-r leaks' repairs only cluster leaks, whereas '-r all' fixes all\n"
> @@ -4461,6 +4476,245 @@ out:
> return 0;
> }
>
> +enum ImgBitmapAct {
> + BITMAP_ADD,
> + BITMAP_REMOVE,
> + BITMAP_CLEAR,
> + BITMAP_ENABLE,
> + BITMAP_DISABLE,
> + BITMAP_MERGE,
> +};
> +typedef struct ImgBitmapAction {
> + enum ImgBitmapAct act;
> + const char *src; /* only used for merge */
> + QSIMPLEQ_ENTRY(ImgBitmapAction) next;
> +} ImgBitmapAction;
> +
> +static int img_bitmap(int argc, char **argv)
> +{
> + Error *err = NULL;
> + int c, ret = -1;
> + QemuOpts *opts = NULL;
> + const char *fmt = NULL, *src_fmt = NULL, *src_filename = NULL;
> + const char *filename, *bitmap;
> + BlockBackend *blk = NULL, *src = NULL;
> + BlockDriverState *bs = NULL, *src_bs = NULL;
> + bool image_opts = false;
> + int64_t granularity = 0;
> + bool add = false, merge = false;
> + QSIMPLEQ_HEAD(, ImgBitmapAction) actions;
> + ImgBitmapAction *act, *act_next;
> + const char *op;
> +
> + QSIMPLEQ_INIT(&actions);
> +
> + for (;;) {
> + static const struct option long_options[] = {
> + {"help", no_argument, 0, 'h'},
> + {"object", required_argument, 0, OPTION_OBJECT},
> + {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
> + {"add", no_argument, 0, OPTION_ADD},
> + {"remove", no_argument, 0, OPTION_REMOVE},
> + {"clear", no_argument, 0, OPTION_CLEAR},
> + {"enable", no_argument, 0, OPTION_ENABLE},
> + {"disable", no_argument, 0, OPTION_DISABLE},
> + {"merge", required_argument, 0, OPTION_MERGE},
> + {"granularity", required_argument, 0, 'g'},
> + {"source-file", required_argument, 0, 'b'},
> + {"source-format", required_argument, 0, 'F'},
> + {0, 0, 0, 0}
> + };
> + c = getopt_long(argc, argv, ":b:f:F:g:h", long_options, NULL);
> + if (c == -1) {
> + break;
> + }
> +
> + switch (c) {
> + case ':':
> + missing_argument(argv[optind - 1]);
> + break;
> + case '?':
> + unrecognized_option(argv[optind - 1]);
> + break;
> + case 'h':
> + help();
> + break;
> + case 'b':
> + src_filename = optarg;
> + break;
> + case 'f':
> + fmt = optarg;
> + break;
> + case 'F':
> + src_fmt = optarg;
> + break;
> + case 'g':
> + granularity = cvtnum(optarg);
> + if (granularity < 0) {
> + error_report("Invalid granularity specified");
> + return 1;
> + }
> + break;
> + case OPTION_ADD:
> + act = g_new0(ImgBitmapAction, 1);
> + act->act = BITMAP_ADD;
> + QSIMPLEQ_INSERT_TAIL(&actions, act, next);
> + add = true;
> + break;
> + case OPTION_REMOVE:
> + act = g_new0(ImgBitmapAction, 1);
> + act->act = BITMAP_REMOVE;
> + QSIMPLEQ_INSERT_TAIL(&actions, act, next);
> + break;
> + case OPTION_CLEAR:
> + act = g_new0(ImgBitmapAction, 1);
> + act->act = BITMAP_CLEAR;
> + QSIMPLEQ_INSERT_TAIL(&actions, act, next);
> + break;
> + case OPTION_ENABLE:
> + act = g_new0(ImgBitmapAction, 1);
> + act->act = BITMAP_ENABLE;
> + QSIMPLEQ_INSERT_TAIL(&actions, act, next);
> + break;
> + case OPTION_DISABLE:
> + act = g_new0(ImgBitmapAction, 1);
> + act->act = BITMAP_DISABLE;
> + QSIMPLEQ_INSERT_TAIL(&actions, act, next);
> + break;
> + case OPTION_MERGE:
> + act = g_new0(ImgBitmapAction, 1);
> + act->act = BITMAP_MERGE;
> + act->src = optarg;
> + QSIMPLEQ_INSERT_TAIL(&actions, act, next);
> + merge = true;
> + break;
> + case OPTION_OBJECT:
> + opts = qemu_opts_parse_noisily(&qemu_object_opts, optarg, true);
> + if (!opts) {
> + goto out;
> + }
> + break;
> + case OPTION_IMAGE_OPTS:
> + image_opts = true;
> + break;
> + }
> + }
> +
> + if (qemu_opts_foreach(&qemu_object_opts,
> + user_creatable_add_opts_foreach,
> + qemu_img_object_print_help, &error_fatal)) {
> + goto out;
> + }
> +
> + if (QSIMPLEQ_EMPTY(&actions)) {
> + error_report("Need at least one of --add, --remove, --clear, "
> + "--enable, --disable, or --merge");
> + goto out;
> + }
> +
> + if (granularity && !add) {
> + error_report("granularity only supported with --add");
> + goto out;
> + }
> + if (src_fmt && !src_filename) {
> + error_report("-F only supported with -b");
> + goto out;
> + }
> + if (src_filename && !merge) {
> + error_report("Merge bitmap source file only supported with "
> + "--merge");
> + goto out;
> + }
> +
> + if (optind != argc - 2) {
> + error_report("Expecting filename and bitmap name");
> + goto out;
> + }
> +
> + filename = argv[optind];
> + bitmap = argv[optind + 1];
> +
> + blk = img_open(image_opts, filename, fmt, BDRV_O_RDWR, false, false,
> + false);
fit in one line
> + if (!blk) {
> + goto out;
> + }
> + bs = blk_bs(blk);
> + if (src_filename) {
> + src = img_open(NULL, src_filename, src_fmt, 0, false, false,
> + false);
s/NULL/false/
also, fit in one line
> + if (!src) {
> + goto out;
> + }
> + src_bs = blk_bs(src);
> + } else {
> + src_bs = bs;
> + }
> +
> + QSIMPLEQ_FOREACH_SAFE(act, &actions, next, act_next) {
> + switch (act->act) {
> + case BITMAP_ADD:
> + qmp_block_dirty_bitmap_add(bs->node_name, bitmap,
> + !!granularity, granularity, true, true,
> + false, false, &err);
> + op = "add";
> + break;
> + case BITMAP_REMOVE:
> + qmp_block_dirty_bitmap_remove(bs->node_name, bitmap, &err);
> + op = "remove";
> + break;
> + case BITMAP_CLEAR:
> + qmp_block_dirty_bitmap_clear(bs->node_name, bitmap, &err);
> + op = "clear";
> + break;
> + case BITMAP_ENABLE:
> + qmp_block_dirty_bitmap_enable(bs->node_name, bitmap, &err);
> + op = "enable";
> + break;
> + case BITMAP_DISABLE:
> + qmp_block_dirty_bitmap_disable(bs->node_name, bitmap, &err);
> + op = "disable";
> + break;
> + case BITMAP_MERGE: {
> + BlockDirtyBitmapMergeSource *merge_src;
> + BlockDirtyBitmapMergeSourceList *list;
> +
> + merge_src = g_new0(BlockDirtyBitmapMergeSource, 1);
> + merge_src->type = QTYPE_QDICT;
> + merge_src->u.external.node = g_strdup(src_bs->node_name);
> + merge_src->u.external.name = g_strdup(act->src);
> + list = g_new0(BlockDirtyBitmapMergeSourceList, 1);
> + list->value = merge_src;
> + qmp_block_dirty_bitmap_merge(bs->node_name, bitmap, list, &err);
> + qapi_free_BlockDirtyBitmapMergeSourceList(list);
> + op = "merge";
> + break;
> + }
> + default:
> + g_assert_not_reached();
> + }
> +
> + if (err) {
> + error_reportf_err(err, "Operation %s on bitmap %s failed",
s/failed/failed: /
> + op, bitmap);
> + ret = -1;
dead assignment: you never set ret after first initialization to -1.
> + goto out;
> + }
> + g_free(act);
> + }
> +
> + ret = 0;
> +
> + out:
> + blk_unref(src);
> + blk_unref(blk);
> + qemu_opts_del(opts);
> + if (ret) {
> + return 1;
> + }
> + return 0;
Hmm, as it's the only usage of ret, you may initialize it to 1 at function start, and here just "return ret;"
> +}
> +
> #define C_BS 01
> #define C_COUNT 02
> #define C_IF 04
> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index e0886437b1f2..011688245668 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -20,6 +20,13 @@ DEF("bench", img_bench,
> SRST
> .. option:: bench [-c COUNT] [-d DEPTH] [-f FMT] [--flush-interval=FLUSH_INTERVAL] [-i AIO] [-n] [--no-drain] [-o OFFSET] [--pattern=PATTERN] [-q] [-s BUFFER_SIZE] [-S STEP_SIZE] [-t CACHE] [-w] [-U] FILENAME
> ERST
> +
> +DEF("bitmap", img_bitmap,
> + "bitmap (--merge SOURCE | --add | --remove | --clear | --enable | --disable)... [-b source_file [-F source_fmt]] [-g granularity] [--object objectdef] [--image-opts | -f fmt] filename bitmap")
> +SRST
> +.. 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
Not about this patch, but it's a pity that we have triple duplications (triplications ?) of such lines..
> +ERST
> +
> DEF("check", img_check,
> "check [--object objectdef] [--image-opts] [-q] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] [-U] filename")
> SRST
>
With at least s/NULL/false/ and s/failed/failed: / (or with all my tiny suggestions):
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
On 5/18/20 6:42 AM, Vladimir Sementsov-Ogievskiy wrote:
> 13.05.2020 04:16, Eric Blake wrote:
>> Include actions for --add, --remove, --clear, --enable, --disable, and
>> --merge (note that --clear is a bit of fluff, because the same can be
>> accomplished by removing a bitmap and then adding a new one in its
>> place, but it matches what QMP commands exist). Listing is omitted,
>> because it does not require a bitmap name and because it was already
>> possible with 'qemu-img info'. A single command line can play one or
>> more bitmap commands in sequence on the same bitmap name (although all
>> added bitmaps share the same granularity, and and all merged bitmaps
>> come from the same source file). Merge defaults to other bitmaps in
>> the primary image, but can also be told to merge bitmaps from a
>> distinct image.
>>
>> While this supports --image-opts for the file being modified, I did
>> not think it worth the extra complexity to support that for the source
>> file in a cross-file merges. Likewise, I chose to have --merge only
>> take a single source rather than following the QMP support for
>> multiple merges in one go (although you can still use more than one
>> --merge in the command line); in part because qemu-img is offline and
>> therefore atomicity is not an issue.
>>
>> +
>> + blk = img_open(image_opts, filename, fmt, BDRV_O_RDWR, false, false,
>> + false);
>
> fit in one line
That line would be exactly 80; I tend to wrap at 79 or earlier rather
than exactly on 80.
>
>> + if (!blk) {
>> + goto out;
>> + }
>> + bs = blk_bs(blk);
>> + if (src_filename) {
>> + src = img_open(NULL, src_filename, src_fmt, 0, false, false,
>> + false);
>
> s/NULL/false/
D'oh. And yet it still compiles. Fixing.
>
> also, fit in one line
Yes, this one's shorter. Fixing.
>> +
>> + if (err) {
>> + error_reportf_err(err, "Operation %s on bitmap %s failed",
>
> s/failed/failed: /
>
>> + op, bitmap);
>> + ret = -1;
>
> dead assignment: you never set ret after first initialization to -1.
Fixing both.
>
>> + goto out;
>> + }
>> + g_free(act);
>> + }
>> +
>> + ret = 0;
>> +
>> + out:
>> + blk_unref(src);
>> + blk_unref(blk);
>> + qemu_opts_del(opts);
>> + if (ret) {
>> + return 1;
>> + }
>> + return 0;
>
> Hmm, as it's the only usage of ret, you may initialize it to 1 at
> function start, and here just "return ret;"
Yep, done.
>> +DEF("bitmap", img_bitmap,
>> + "bitmap (--merge SOURCE | --add | --remove | --clear | --enable |
>> --disable)... [-b source_file [-F source_fmt]] [-g granularity]
>> [--object objectdef] [--image-opts | -f fmt] filename bitmap")
>> +SRST
>> +.. 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
>
> Not about this patch, but it's a pity that we have triple duplications
> (triplications ?) of such lines..
Yes, it is annoying. But as you say, it's a cleanup for another day,
for someone who is interested.
>
> With at least s/NULL/false/ and s/failed/failed: / (or with all my tiny
> suggestions):
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Thanks; squashing in:
diff --git i/qemu-img.c w/qemu-img.c
index 8c99e68ba8aa..f940137cb0e5 100644
--- i/qemu-img.c
+++ w/qemu-img.c
@@ -4493,7 +4493,7 @@ typedef struct ImgBitmapAction {
static int img_bitmap(int argc, char **argv)
{
Error *err = NULL;
- int c, ret = -1;
+ int c, ret = 1;
QemuOpts *opts = NULL;
const char *fmt = NULL, *src_fmt = NULL, *src_filename = NULL;
const char *filename, *bitmap;
@@ -4641,8 +4641,7 @@ static int img_bitmap(int argc, char **argv)
}
bs = blk_bs(blk);
if (src_filename) {
- src = img_open(NULL, src_filename, src_fmt, 0, false, false,
- false);
+ src = img_open(false, src_filename, src_fmt, 0, false, false,
false);
if (!src) {
goto out;
}
@@ -4695,9 +4694,8 @@ static int img_bitmap(int argc, char **argv)
}
if (err) {
- error_reportf_err(err, "Operation %s on bitmap %s failed",
+ error_reportf_err(err, "Operation %s on bitmap %s failed: ",
op, bitmap);
- ret = -1;
goto out;
}
g_free(act);
@@ -4709,10 +4707,7 @@ static int img_bitmap(int argc, char **argv)
blk_unref(src);
blk_unref(blk);
qemu_opts_del(opts);
- if (ret) {
- return 1;
- }
- return 0;
+ return ret;
}
#define C_BS 01
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
18.05.2020 22:07, Eric Blake wrote:
> On 5/18/20 6:42 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 13.05.2020 04:16, Eric Blake wrote:
>>> Include actions for --add, --remove, --clear, --enable, --disable, and
>>> --merge (note that --clear is a bit of fluff, because the same can be
>>> accomplished by removing a bitmap and then adding a new one in its
>>> place, but it matches what QMP commands exist). Listing is omitted,
>>> because it does not require a bitmap name and because it was already
>>> possible with 'qemu-img info'. A single command line can play one or
>>> more bitmap commands in sequence on the same bitmap name (although all
>>> added bitmaps share the same granularity, and and all merged bitmaps
>>> come from the same source file). Merge defaults to other bitmaps in
>>> the primary image, but can also be told to merge bitmaps from a
>>> distinct image.
>>>
>>> While this supports --image-opts for the file being modified, I did
>>> not think it worth the extra complexity to support that for the source
>>> file in a cross-file merges. Likewise, I chose to have --merge only
>>> take a single source rather than following the QMP support for
>>> multiple merges in one go (although you can still use more than one
>>> --merge in the command line); in part because qemu-img is offline and
>>> therefore atomicity is not an issue.
>>>
>
>>> +
>>> + blk = img_open(image_opts, filename, fmt, BDRV_O_RDWR, false, false,
>>> + false);
>>
>> fit in one line
>
> That line would be exactly 80; I tend to wrap at 79 or earlier rather than exactly on 80.
>
>>
>>> + if (!blk) {
>>> + goto out;
>>> + }
>>> + bs = blk_bs(blk);
>>> + if (src_filename) {
>>> + src = img_open(NULL, src_filename, src_fmt, 0, false, false,
>>> + false);
>>
>> s/NULL/false/
>
> D'oh. And yet it still compiles. Fixing.
>
>>
>> also, fit in one line
>
> Yes, this one's shorter. Fixing.
>
>
>>> +
>>> + if (err) {
>>> + error_reportf_err(err, "Operation %s on bitmap %s failed",
>>
>> s/failed/failed: /
>>
>>> + op, bitmap);
>>> + ret = -1;
>>
>> dead assignment: you never set ret after first initialization to -1.
>
> Fixing both.
>
>>
>>> + goto out;
>>> + }
>>> + g_free(act);
>>> + }
>>> +
>>> + ret = 0;
>>> +
>>> + out:
>>> + blk_unref(src);
>>> + blk_unref(blk);
>>> + qemu_opts_del(opts);
>>> + if (ret) {
>>> + return 1;
>>> + }
>>> + return 0;
>>
>> Hmm, as it's the only usage of ret, you may initialize it to 1 at function start, and here just "return ret;"
>
> Yep, done.
>
>>> +DEF("bitmap", img_bitmap,
>>> + "bitmap (--merge SOURCE | --add | --remove | --clear | --enable | --disable)... [-b source_file [-F source_fmt]] [-g granularity] [--object objectdef] [--image-opts | -f fmt] filename bitmap")
>>> +SRST
>>> +.. 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
>>
>> Not about this patch, but it's a pity that we have triple duplications (triplications ?) of such lines..
>
> Yes, it is annoying. But as you say, it's a cleanup for another day, for someone who is interested.
>
>>
>> With at least s/NULL/false/ and s/failed/failed: / (or with all my tiny suggestions):
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Thanks; squashing in:
>
> diff --git i/qemu-img.c w/qemu-img.c
> index 8c99e68ba8aa..f940137cb0e5 100644
> --- i/qemu-img.c
> +++ w/qemu-img.c
> @@ -4493,7 +4493,7 @@ typedef struct ImgBitmapAction {
> static int img_bitmap(int argc, char **argv)
> {
> Error *err = NULL;
> - int c, ret = -1;
> + int c, ret = 1;
> QemuOpts *opts = NULL;
> const char *fmt = NULL, *src_fmt = NULL, *src_filename = NULL;
> const char *filename, *bitmap;
> @@ -4641,8 +4641,7 @@ static int img_bitmap(int argc, char **argv)
> }
> bs = blk_bs(blk);
> if (src_filename) {
> - src = img_open(NULL, src_filename, src_fmt, 0, false, false,
> - false);
> + src = img_open(false, src_filename, src_fmt, 0, false, false, false);
> if (!src) {
> goto out;
> }
> @@ -4695,9 +4694,8 @@ static int img_bitmap(int argc, char **argv)
> }
>
> if (err) {
> - error_reportf_err(err, "Operation %s on bitmap %s failed",
> + error_reportf_err(err, "Operation %s on bitmap %s failed: ",
> op, bitmap);
> - ret = -1;
> goto out;
> }
> g_free(act);
> @@ -4709,10 +4707,7 @@ static int img_bitmap(int argc, char **argv)
> blk_unref(src);
> blk_unref(blk);
> qemu_opts_del(opts);
> - if (ret) {
> - return 1;
> - }
> - return 0;
> + return ret;
> }
>
> #define C_BS 01
>
Great, keep my r-b with this.
--
Best regards,
Vladimir
© 2016 - 2026 Red Hat, Inc.