Re: [PATCH v4 6/9] qemu-img: Add bitmap sub-command

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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