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

[Eric Blake]

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

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