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

[Max Reitz]

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

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

signature.asc
Description: OpenPGP digital signature