qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 3/6] qemu-img: Add bitmap sub-command


From: Max Reitz
Subject: Re: [PATCH v2 3/6] qemu-img: Add bitmap sub-command
Date: Thu, 30 Apr 2020 16:55:13 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0

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 <address@hidden>
> ---
>  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

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]