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

[Eric Blake <eblake@redhat.com>]

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