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'.  Merge can work either from another
bitmap in the same image, or from a bitmap in 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 bitmap merge.  Likewise, I chose to have --merge
only take a single source rather than following the QMP support for
multiple merges in one go; in part to simplify the command line, and
in part because an offline image can achieve the same effect by
multiple qemu-img bitmap --merge calls.  We can enhance that if needed
in the future (the same way that 'qemu-img convert' has a mode that
concatenates multiple sources into one destination).

Upcoming patches will add iotest coverage of these commands while
also testing other features.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/tools/qemu-img.rst |  24 +++++
 qemu-img.c              | 198 ++++++++++++++++++++++++++++++++++++++++
 qemu-img-cmds.hx        |   7 ++
 3 files changed, 229 insertions(+)

diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 7d08c48d308f..4f3b0e2c9ace 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 {--add [-g GRANULARITY] [--disabled] | --remove | --clear | --enable | --disable | --merge SOURCE_BITMAP [-b SOURCE_FILE [-F SOURCE_FMT]]} [--object OBJECTDEF] [--image-opts] [-f FMT] FILENAME BITMAP
+
+  Perform a modification of the persistent bitmap *BITMAP* in the disk
+  image *FILENAME*.  The various modifications are:
+
+  ``--add`` to create *BITMAP*, with additional options ``-g`` to
+  specify a non-default *GRANULARITY*, or whether the bitmap should be
+  ``--disabled`` instead of enabled.
+
+  ``--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*.
+  This defaults to requiring a source bitmap from the same *FILENAME*,
+  but can also be used for cross-image merge by supplying ``-b`` to
+  specify a different *SOURCE_FILE*.
+
+  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 821cbf610e5f..02ebd870faa1 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 {
@@ -4438,6 +4445,197 @@ out:
     return 0;
 }

+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;
+    unsigned long granularity = 0;
+    bool add = false, remove = false, clear = false;
+    bool enable = false, disable = false, add_disabled = false;
+    const char *merge = NULL;
+
+    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},
+            {"disabled", 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':
+            if (qemu_strtosz(optarg, NULL, &granularity)) {
+                error_report("Invalid granularity specified");
+                return 1;
+            }
+            break;
+        case OPTION_ADD:
+            add = true;
+            break;
+        case OPTION_REMOVE:
+            remove = true;
+            break;
+        case OPTION_CLEAR:
+            clear = true;
+            break;
+        case OPTION_ENABLE:
+            enable = true;
+            break;
+        case OPTION_DISABLE:
+            disable = true;
+            break;
+        case OPTION_MERGE:
+            merge = optarg;
+            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 (add && disable) {
+        disable = false;
+        add_disabled = true;
+    }
+    if (add + remove + clear + enable + disable + !!merge != 1) {
+        error_report("Need exactly one mode 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("alternate 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 (add) {
+        qmp_block_dirty_bitmap_add(bs->node_name, bitmap,
+                                   !!granularity, granularity, true, true,
+                                   true, add_disabled, &err);
+    } else if (remove) {
+        qmp_block_dirty_bitmap_remove(bs->node_name, bitmap, &err);
+    } else if (clear) {
+        qmp_block_dirty_bitmap_clear(bs->node_name, bitmap, &err);
+    } else if (enable) {
+        qmp_block_dirty_bitmap_enable(bs->node_name, bitmap, &err);
+    } else if (disable) {
+        qmp_block_dirty_bitmap_disable(bs->node_name, bitmap, &err);
+    } else if (merge) {
+        BlockDirtyBitmapMergeSource *merge_src;
+        BlockDirtyBitmapMergeSourceList *list;
+
+        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;
+        }
+
+        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(merge);
+        list = g_new0(BlockDirtyBitmapMergeSourceList, 1);
+        list->value = merge_src;
+        qmp_block_dirty_bitmap_merge(bs->node_name, bitmap, list, &err);
+        qapi_free_BlockDirtyBitmapMergeSourceList(list);
+    }
+
+    if (err) {
+        error_reportf_err(err, "Bitmap %s operation failed", bitmap);
+        ret = -1;
+        goto out;
+    }
+
+    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 c9c54de1df40..bf0035e226c8 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 {--add [-g granularity] [--disabled] | --remove | --clear | --enable | --disable | --merge source_bitmap [-b source_file [-F source_fmt]]} [--object objectdef] [--image-opts] [-f fmt] filename bitmap")
+SRST
+.. option:: bitmap {--add [-g GRANULARITY] [--disabled] | --remove | --clear | --enable | --disable | --merge SOURCE_BITMAP [-b SOURCE_FILE [-F SOURCE_FMT]]} [--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

On 21.04.20 23:20, 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,

Well, ideally, all of qemu-img is just fluff because “the same can be
accomplished by launching qemu and issuing the equivalent QMP commands”. :)

>        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'.

Fair enough, although it can be said that qemu-img info’s output is
qcow2-specific.  It might be nice to have some definitely
format-independent output.  (But we don’t have persistent bitmaps in
anything but qcow2 yet (or do we in NBD?), so I don’t expect anyone to
care much.)

>                                 Merge can work either from another
> bitmap in the same image, or from a bitmap in 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 bitmap merge.  Likewise, I chose to have --merge
> only take a single source rather than following the QMP support for
> multiple merges in one go; in part to simplify the command line, and
> in part because an offline image can achieve the same effect by
> multiple qemu-img bitmap --merge calls.  We can enhance that if needed
> in the future (the same way that 'qemu-img convert' has a mode that
> concatenates multiple sources into one destination).
> 
> Upcoming patches will add iotest coverage of these commands while
> also testing other features.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  docs/tools/qemu-img.rst |  24 +++++
>  qemu-img.c              | 198 ++++++++++++++++++++++++++++++++++++++++
>  qemu-img-cmds.hx        |   7 ++
>  3 files changed, 229 insertions(+)
> 
> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index 7d08c48d308f..4f3b0e2c9ace 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 {--add [-g GRANULARITY] [--disabled] | --remove | --clear | --enable | --disable | --merge SOURCE_BITMAP [-b SOURCE_FILE [-F SOURCE_FMT]]} [--object OBJECTDEF] [--image-opts] [-f FMT] FILENAME BITMAP

So I can do multiple operations in one roll, but they all use the same
BITMAP?  Sounds a bit weird.  It actually took me a while to understands
this, because I thought for sure that each command would take a bitmap
name.  (And was ready to complain that it looked like they don’t, but,
well, that’s because they don’t.)

Although I suppose some practical example like

$ qemu-img bitmap --add --merge sbmap --disable foo.qcow2 nbmap

does make sense.[1]


Would

$ qemu-img bitmap --add nbmap --merge nbmap sbmap --enable nbmap \
      foo.qcow2

make more sense?  Doesn’t really look like it, because at that point you
just have to ask why not just call qemu-img bitmap multiple times.

*shrug*


[1] However, that leads me to ask:  Why does --add need a --disabled
option?  Isn’t that equivalent to --add --disable?

> +
> +  Perform a modification of the persistent bitmap *BITMAP* in the disk
> +  image *FILENAME*.  The various modifications are:
> +
> +  ``--add`` to create *BITMAP*, with additional options ``-g`` to
> +  specify a non-default *GRANULARITY*, or whether the bitmap should be
> +  ``--disabled`` instead of enabled.
> +
> +  ``--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*.
> +  This defaults to requiring a source bitmap from the same *FILENAME*,
> +  but can also be used for cross-image merge by supplying ``-b`` to
> +  specify a different *SOURCE_FILE*.
> +
> +  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 821cbf610e5f..02ebd870faa1 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 {
> @@ -4438,6 +4445,197 @@ out:
>      return 0;
>  }
> 
> +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;
> +    unsigned long granularity = 0;
> +    bool add = false, remove = false, clear = false;
> +    bool enable = false, disable = false, add_disabled = false;
> +    const char *merge = NULL;

So actually we can’t do one operation multiple times in one roll.

> +
> +    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},
> +            {"disabled", no_argument, 0, OPTION_DISABLE},

So if --disable and --disabled are exactly the same, I really don’t know
why --disabled even exists.

> +            {"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':
> +            if (qemu_strtosz(optarg, NULL, &granularity)) {
> +                error_report("Invalid granularity specified");
> +                return 1;
> +            }
> +            break;
> +        case OPTION_ADD:
> +            add = true;
> +            break;
> +        case OPTION_REMOVE:
> +            remove = true;
> +            break;
> +        case OPTION_CLEAR:
> +            clear = true;
> +            break;
> +        case OPTION_ENABLE:
> +            enable = true;
> +            break;
> +        case OPTION_DISABLE:
> +            disable = true;
> +            break;
> +        case OPTION_MERGE:
> +            merge = optarg;
> +            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 (add && disable) {
> +        disable = false;
> +        add_disabled = true;
> +    }
> +    if (add + remove + clear + enable + disable + !!merge != 1) {
> +        error_report("Need exactly one mode of --add, --remove, --clear, "
> +                     "--enable, --disable, or --merge");

Aha.  So you can actually only do a single operation.

That means the doc shouldn’t use {}, in my opinion, because that to me
means repetition (thanks to EBNF).  It definitely served to confuse me
greatly until this point.

I also don’t see why we would disallow multiple operations in one go.
The --add --merge combination seems useful to me, and I don’t see a
problem in implementing it.

I don’t know why we don’t just create a list of operations to execute,
based on the order given in the argument list, and then execute them in
order.  That would even allow multiple --merge operations.

If we don’t want that (because we don’t want argument order to matter),
we could still allow all operations to be done at least once and always
execute them in the same order, e.g.:
(1) add
(2) clear
(3) merge
(4) disable
(5) enable
(6) remove

> +        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("alternate source file only supported with --merge");

“alternate” sounds strange.  Why not be more precise and call it a
“Merge bitmap source file” or “File to source merge bitmap from”?

> +        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 (add) {
> +        qmp_block_dirty_bitmap_add(bs->node_name, bitmap,
> +                                   !!granularity, granularity, true, true,
> +                                   true, add_disabled, &err);
> +    } else if (remove) {
> +        qmp_block_dirty_bitmap_remove(bs->node_name, bitmap, &err);
> +    } else if (clear) {
> +        qmp_block_dirty_bitmap_clear(bs->node_name, bitmap, &err);
> +    } else if (enable) {
> +        qmp_block_dirty_bitmap_enable(bs->node_name, bitmap, &err);
> +    } else if (disable) {
> +        qmp_block_dirty_bitmap_disable(bs->node_name, bitmap, &err);
> +    } else if (merge) {
> +        BlockDirtyBitmapMergeSource *merge_src;
> +        BlockDirtyBitmapMergeSourceList *list;
> +
> +        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;
> +        }
> +
> +        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(merge);
> +        list = g_new0(BlockDirtyBitmapMergeSourceList, 1);
> +        list->value = merge_src;
> +        qmp_block_dirty_bitmap_merge(bs->node_name, bitmap, list, &err);
> +        qapi_free_BlockDirtyBitmapMergeSourceList(list);
> +    }
> +
> +    if (err) {
> +        error_reportf_err(err, "Bitmap %s operation failed", bitmap);

Wouldn’t “Operation on bitmap %s failed” work better?

Max

> +        ret = -1;
> +        goto out;
> +    }
> +
> +    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

On 4/30/20 9:55 AM, Max Reitz wrote:
> On 21.04.20 23:20, 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,
> 
> Well, ideally, all of qemu-img is just fluff because “the same can be
> accomplished by launching qemu and issuing the equivalent QMP commands”. :)
> 
>>         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'.
> 
> Fair enough, although it can be said that qemu-img info’s output is
> qcow2-specific.  It might be nice to have some definitely
> format-independent output.  (But we don’t have persistent bitmaps in
> anything but qcow2 yet (or do we in NBD?), so I don’t expect anyone to
> care much.)

We can add a list subcommand later if it is still desired.  I agree that 
a tabular format:

name          enabled   granularity
bitmap1       false     65536
bitmap2       true      512

in isolation is easier to read than:

     bitmaps:
         [0]:
             flags:
             name: bitmap1
             granularity: 65536
         [1]:
             flags:
                 [0]: auto
             name: bitmap2
             granularity: 512

embedded inside even more information.

> 
>>                                  Merge can work either from another
>> bitmap in the same image, or from a bitmap in 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 bitmap merge.  Likewise, I chose to have --merge
>> only take a single source rather than following the QMP support for
>> multiple merges in one go; in part to simplify the command line, and
>> in part because an offline image can achieve the same effect by
>> multiple qemu-img bitmap --merge calls.  We can enhance that if needed
>> in the future (the same way that 'qemu-img convert' has a mode that
>> concatenates multiple sources into one destination).
>>
>> Upcoming patches will add iotest coverage of these commands while
>> also testing other features.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>   docs/tools/qemu-img.rst |  24 +++++
>>   qemu-img.c              | 198 ++++++++++++++++++++++++++++++++++++++++
>>   qemu-img-cmds.hx        |   7 ++
>>   3 files changed, 229 insertions(+)
>>
>> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
>> index 7d08c48d308f..4f3b0e2c9ace 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 {--add [-g GRANULARITY] [--disabled] | --remove | --clear | --enable | --disable | --merge SOURCE_BITMAP [-b SOURCE_FILE [-F SOURCE_FMT]]} [--object OBJECTDEF] [--image-opts] [-f FMT] FILENAME BITMAP
> 
> So I can do multiple operations in one roll, but they all use the same
> BITMAP?  Sounds a bit weird.  It actually took me a while to understands
> this, because I thought for sure that each command would take a bitmap
> name.  (And was ready to complain that it looked like they don’t, but,
> well, that’s because they don’t.)

All of the operations take one bitmap name (the final BITMAP). 
Additionally, the --merge operation takes a second bitmap name 
(SOURCE_BITMAP).  None of the other operations need a second bitmap 
name, so only --merge requires an option argument.  As written, the { a 
| b | c } implies that operations are mutually exclusive: you can only 
request one operation per qemu-img invocation.

> 
> Although I suppose some practical example like
> 
> $ qemu-img bitmap --add --merge sbmap --disable foo.qcow2 nbmap
> 
> does make sense.[1]
> 
> 
> Would
> 
> $ qemu-img bitmap --add nbmap --merge nbmap sbmap --enable nbmap \
>        foo.qcow2
> 
> make more sense?

That would be more transactional, and more effort to implement.  My 
argument is that you should instead write:

$ qemu-img bitmap --add foo.qcow2 nbmap
$ qemu-img bitmap --merge sbmap foo.qcow2 nbmap
$ qemu-img bitmap --enable foo.qcow2 nbmap

where I only have to implement one operation per qemu-img.

>    Doesn’t really look like it, because at that point you
> just have to ask why not just call qemu-img bitmap multiple times.
> 
> *shrug*
> 
> 
> [1] However, that leads me to ask:  Why does --add need a --disabled
> option?  Isn’t that equivalent to --add --disable?

The QMP command for add has an optional 'disabled' parameter, which I 
exposed here.  Alternatively, we could indeed NOT expose that parameter 
through qemu-img, but require two separate qemu-img bitmap commands to 
add and then disable things.  Atomicity is not a concern here like it 
was in QMP.  Removing that sugar does simplify things slightly.


>> +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;
>> +    unsigned long granularity = 0;
>> +    bool add = false, remove = false, clear = false;
>> +    bool enable = false, disable = false, add_disabled = false;
>> +    const char *merge = NULL;
> 
> So actually we can’t do one operation multiple times in one roll.

Correct.  At least, not as I wrote it.

> 
>> +
>> +    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},
>> +            {"disabled", no_argument, 0, OPTION_DISABLE},
> 
> So if --disable and --disabled are exactly the same, I really don’t know
> why --disabled even exists.

Logically, '--add --disabled' matches the name of the QMP command with 
its optional 'disabled' parameter, while --disable matches the name of 
the QMP command.  We don't have to have the alias; and in fact, if you 
agree that supporting '--add --disabled' is too much sugar (since we 
don't care about atomicity the way QMP did), then life gets simpler to 
drop --disabled altogether.

>> +    if (add && disable) {
>> +        disable = false;
>> +        add_disabled = true;
>> +    }
>> +    if (add + remove + clear + enable + disable + !!merge != 1) {
>> +        error_report("Need exactly one mode of --add, --remove, --clear, "
>> +                     "--enable, --disable, or --merge");
> 
> Aha.  So you can actually only do a single operation.
> 
> That means the doc shouldn’t use {}, in my opinion, because that to me
> means repetition (thanks to EBNF).  It definitely served to confuse me
> greatly until this point.

In command line syntax, I'm most used to seeing repetition as '...', 
optional as [], and mutually-exclusive choice as {|}.  Yes, that's 
different than EBNF.

> 
> I also don’t see why we would disallow multiple operations in one go.
> The --add --merge combination seems useful to me, and I don’t see a
> problem in implementing it.
> 
> I don’t know why we don’t just create a list of operations to execute,
> based on the order given in the argument list, and then execute them in
> order.  That would even allow multiple --merge operations.

If I understand, you're asking why we can't do:

qemu-img bitmap foo.qcow2 --add b1 --merge sb b1

in one operation.

That changes the syntax entirely, compared to what I implemented.  You'd 
have to have an argument to every option, AND figure out how to specify 
TWO arguments to the --merge option.  Might be doable, but seems hairy.

> 
> If we don’t want that (because we don’t want argument order to matter),
> we could still allow all operations to be done at least once and always
> execute them in the same order, e.g.:
> (1) add
> (2) clear
> (3) merge
> (4) disable
> (5) enable
> (6) remove

I still find it simpler to do exactly one operation per invocation.

> 
>> +        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("alternate source file only supported with --merge");
> 
> “alternate” sounds strange.  Why not be more precise and call it a
> “Merge bitmap source file” or “File to source merge bitmap from”?

"Merge bitmap source file" sounds fine.

>> +
>> +    if (err) {
>> +        error_reportf_err(err, "Bitmap %s operation failed", bitmap);
> 
> Wouldn’t “Operation on bitmap %s failed” work better?

Yes.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 30.04.20 17:21, Eric Blake wrote:
> On 4/30/20 9:55 AM, Max Reitz wrote:
>> On 21.04.20 23:20, 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,
>>
>> Well, ideally, all of qemu-img is just fluff because “the same can be
>> accomplished by launching qemu and issuing the equivalent QMP
>> commands”. :)
>>
>>>         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'.
>>
>> Fair enough, although it can be said that qemu-img info’s output is
>> qcow2-specific.  It might be nice to have some definitely
>> format-independent output.  (But we don’t have persistent bitmaps in
>> anything but qcow2 yet (or do we in NBD?), so I don’t expect anyone to
>> care much.)
> 
> We can add a list subcommand later if it is still desired.  I agree that
> a tabular format:
> 
> name          enabled   granularity
> bitmap1       false     65536
> bitmap2       true      512
> 
> in isolation is easier to read than:
> 
>     bitmaps:
>         [0]:
>             flags:
>             name: bitmap1
>             granularity: 65536
>         [1]:
>             flags:
>                 [0]: auto
>             name: bitmap2
>             granularity: 512
> 
> embedded inside even more information.
> 
>>
>>>                                  Merge can work either from another
>>> bitmap in the same image, or from a bitmap in 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 bitmap merge.  Likewise, I chose to have --merge
>>> only take a single source rather than following the QMP support for
>>> multiple merges in one go; in part to simplify the command line, and
>>> in part because an offline image can achieve the same effect by
>>> multiple qemu-img bitmap --merge calls.  We can enhance that if needed
>>> in the future (the same way that 'qemu-img convert' has a mode that
>>> concatenates multiple sources into one destination).
>>>
>>> Upcoming patches will add iotest coverage of these commands while
>>> also testing other features.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> ---
>>>   docs/tools/qemu-img.rst |  24 +++++
>>>   qemu-img.c              | 198 ++++++++++++++++++++++++++++++++++++++++
>>>   qemu-img-cmds.hx        |   7 ++
>>>   3 files changed, 229 insertions(+)
>>>
>>> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
>>> index 7d08c48d308f..4f3b0e2c9ace 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 {--add [-g GRANULARITY] [--disabled] | --remove |
>>> --clear | --enable | --disable | --merge SOURCE_BITMAP [-b
>>> SOURCE_FILE [-F SOURCE_FMT]]} [--object OBJECTDEF] [--image-opts] [-f
>>> FMT] FILENAME BITMAP
>>
>> So I can do multiple operations in one roll, but they all use the same
>> BITMAP?  Sounds a bit weird.  It actually took me a while to understands
>> this, because I thought for sure that each command would take a bitmap
>> name.  (And was ready to complain that it looked like they don’t, but,
>> well, that’s because they don’t.)
> 
> All of the operations take one bitmap name (the final BITMAP).
> Additionally, the --merge operation takes a second bitmap name
> (SOURCE_BITMAP).  None of the other operations need a second bitmap
> name, so only --merge requires an option argument.  As written, the { a
> | b | c } implies that operations are mutually exclusive: you can only
> request one operation per qemu-img invocation.

Well, as I found out later it’s supposed to imply that.  I always expect
{} to mean repetition.

>> Although I suppose some practical example like
>>
>> $ qemu-img bitmap --add --merge sbmap --disable foo.qcow2 nbmap
>>
>> does make sense.[1]
>>
>>
>> Would
>>
>> $ qemu-img bitmap --add nbmap --merge nbmap sbmap --enable nbmap \
>>        foo.qcow2
>>
>> make more sense?
> 
> That would be more transactional, and more effort to implement.

As a user, I wouldn’t expect it to be a transaction, but just executed
in order.

> My argument is that you should instead write:
> 
> $ qemu-img bitmap --add foo.qcow2 nbmap
> $ qemu-img bitmap --merge sbmap foo.qcow2 nbmap
> $ qemu-img bitmap --enable foo.qcow2 nbmap
> 
> where I only have to implement one operation per qemu-img.

I don’t know about the “have to”, because from an algorithm standpoint,
doing so would be trivial.  (That is, collecting the operations into a
list, along with their specific arguments, and then execute the list in
a loop.)

I can see that doing this in C is more difficult than it would be in
nicer languages, and so not actually trivial.

But allowing all switches at most once without mutual exclusion still
seems simple.  The question is mostly whether the implementation would
match what we can expect users to expect.

[...]
>> So if --disable and --disabled are exactly the same, I really don’t know
>> why --disabled even exists.
> 
> Logically, '--add --disabled' matches the name of the QMP command with
> its optional 'disabled' parameter, while --disable matches the name of
> the QMP command.  We don't have to have the alias; and in fact, if you
> agree that supporting '--add --disabled' is too much sugar (since we
> don't care about atomicity the way QMP did), then life gets simpler to
> drop --disabled altogether.

I find it makes the interface unnecessarily complex, as does requiring
mutual exclusion.

If we want mutual exclusion, I can see that a separate --disabled for
--add makes sense.

>>> +    if (add && disable) {
>>> +        disable = false;
>>> +        add_disabled = true;
>>> +    }
>>> +    if (add + remove + clear + enable + disable + !!merge != 1) {
>>> +        error_report("Need exactly one mode of --add, --remove,
>>> --clear, "
>>> +                     "--enable, --disable, or --merge");
>>
>> Aha.  So you can actually only do a single operation.
>>
>> That means the doc shouldn’t use {}, in my opinion, because that to me
>> means repetition (thanks to EBNF).  It definitely served to confuse me
>> greatly until this point.
> 
> In command line syntax, I'm most used to seeing repetition as '...',
> optional as [], and mutually-exclusive choice as {|}.  Yes, that's
> different than EBNF.

It’s confusing is what it is, and unnecessarily so.  The | already
signifies exclusion: Say there are -a and -b, if they’re not mutually
exclusive, then the doc describe them as “[-a] [-b]”; if they are, it’d
be “-a | -b”.  Maybe “(-a | -b)”.

I can’t remember having seen {|} for mutual exclusivity before, but then
again, it doesn’t happen often, I suppose.  git for one thing seems to
use (|).

(Regex also uses {} for repetition, even if in a different way from EBNF.)

I’ve never seen “...” used for switches, only for free-form arguments
like filenames.  (Because normally, a switch can be specified only once,
or it wouldn’t be a “switch”.)

>> I also don’t see why we would disallow multiple operations in one go.
>> The --add --merge combination seems useful to me, and I don’t see a
>> problem in implementing it.
>>
>> I don’t know why we don’t just create a list of operations to execute,
>> based on the order given in the argument list, and then execute them in
>> order.  That would even allow multiple --merge operations.
> 
> If I understand, you're asking why we can't do:
> 
> qemu-img bitmap foo.qcow2 --add b1 --merge sb b1
> 
> in one operation.
> 
> That changes the syntax entirely, compared to what I implemented.  You'd
> have to have an argument to every option, AND figure out how to specify
> TWO arguments to the --merge option.  Might be doable, but seems hairy.

Just “qemu-img bitmap --add --merge sb foo.qcow2 b1” would be enough.

>> If we don’t want that (because we don’t want argument order to matter),
>> we could still allow all operations to be done at least once and always
>> execute them in the same order, e.g.:
>> (1) add
>> (2) clear
>> (3) merge
>> (4) disable
>> (5) enable
>> (6) remove
> 
> I still find it simpler to do exactly one operation per invocation.

I feel like that’s mostly because of our coding style allowing an “else
if” to end and start a block on the same line.

(That is, I feel like if we allowed multiple operations in a single go,
the only difference would be that we wouldn’t have to check for mutual
exclusivity, and that all “} else if (cond) {”s would have to be turned
into “} if (cond) {”s.  And reordered.)

It’s possible that I mostly feel compelled to vote for non-exclusivity
because it would be clear then how to document the interface (i.e.,
“[--add] [--clear] [...]”) and I wouldn’t have to explain my utter
confusion at the sight of {}.

Max

On 5/4/20 5:01 AM, Max Reitz wrote:

>>>> +.. option:: bitmap {--add [-g GRANULARITY] [--disabled] | --remove |
>>>> --clear | --enable | --disable | --merge SOURCE_BITMAP [-b
>>>> SOURCE_FILE [-F SOURCE_FMT]]} [--object OBJECTDEF] [--image-opts] [-f
>>>> FMT] FILENAME BITMAP
>>>
>>> So I can do multiple operations in one roll, but they all use the same
>>> BITMAP?  Sounds a bit weird.  It actually took me a while to understands
>>> this, because I thought for sure that each command would take a bitmap
>>> name.  (And was ready to complain that it looked like they don’t, but,
>>> well, that’s because they don’t.)
>>
>> All of the operations take one bitmap name (the final BITMAP).
>> Additionally, the --merge operation takes a second bitmap name
>> (SOURCE_BITMAP).  None of the other operations need a second bitmap
>> name, so only --merge requires an option argument.  As written, the { a
>> | b | c } implies that operations are mutually exclusive: you can only
>> request one operation per qemu-img invocation.
> 
> Well, as I found out later it’s supposed to imply that.  I always expect
> {} to mean repetition.


>> In command line syntax, I'm most used to seeing repetition as '...',
>> optional as [], and mutually-exclusive choice as {|}.  Yes, that's
>> different than EBNF.
> 
> It’s confusing is what it is, and unnecessarily so.  The | already
> signifies exclusion: Say there are -a and -b, if they’re not mutually
> exclusive, then the doc describe them as “[-a] [-b]”; if they are, it’d
> be “-a | -b”.  Maybe “(-a | -b)”.

Is s/{/(/ is all the more that would be needed to take this patch as-is? 
  Or should I really try and re-write it to take a list of operations, 
and iterate through the list until the first failure?

(At any rate, I'll probably end up trying that anyways, just for 
comparison in the lines of code...)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org