Re: [Qemu-devel] [PATCH v5 3/6] add backup related monitor commands

[Markus Armbruster]

Dietmar Maurer <address@hidden> writes:

>> >> +# @devlist: #optional list of block device names (separated by ',', ';'
>> >> +# or ':'). By default the backup includes all writable block devices.
>> >
>> > Make this a proper list, please.
>> 
>> That is, make it a JSON array: '*devlist' : [ 'str' ] Any time that
>> you pass a string
>> through JSON that then requires further ad-hoc parsing (such as
>> splitting on ',' or
>> ':'), it is a sign that your JSON representation was incorrect.
>
> I want to use that on HMP, so I need to parse ad-hoc anyways?

HMP syntax should be designed for humans, QMP syntax for machines.

Example: HMP size and time parameters accept numbers with unit suffixes.
QMP accept only plain numbers.

Example: HMP's netdev_add takes a single argument of the form
KEY=VALUE,...  QMP takes a JSON object { "KEY": "VALUE", ... }.

Your QMP syntax should make use of JSON as much as possible.  In
particular, use JSON numbers for numeric quantities, JSON arrays for
lists, JSON objects for dictionaries.

You may have to do some ad hoc parsing to go from HMP syntax to
QAPI/QMP.  In your particular case: you want a list of device names.
Device names are of the form /[a-z][-._a-z0-9]/i (see id_wellformed()).
Thus, having them in a single argument separated by /[,;:]/ is
syntactically unambiguous.

Three alternative separators are too baroque for my taste.  Even if you
drop all but one, it's still ad hoc syntax.  We've been there, done
that, it's bound to degenerate into inconsistent mess.

Therefore, I'd very much prefer to use generic syntax.  Options I can
see:

1. Define generic syntax for lists of identifiers, in monitor.c.

2. Why new syntax when existing syntax works?  Instead of

        backup foo.vma dev1:dev2:dev3

   have

        backup foo.vma dev1 dev2 dev3

   Requires support for arg_type B* in monitor.c.

>> > You bake the "only one backup at a time" restriction right into the API.
>> > That's fine if and only if multiple backups cannot possibly make sense.
>> >
>> > Unless we agree that's the case, the API needs to be designed so it
>> > can
>> > grow: backup needs to return an ID (it already does), backup-cancel
>> > needs to take it as argument (it doesn't), and query-backup either
>> > returns a list, or takes an ID argument.
>> 
>> Agreed.  In the case of backup-cancel, if you want to make the argument
>> optional, and have it do the right thing when only one job is active
>> (and only fail
>> if multiple jobs are running), that would also work.
>> 
>> >
>> > The implementation may still restrict to a single backup, of course.
>> 
>> The initial implementation, obviously.  The point is that the API
>> should allow for
>> growth, even if the initial implementation doesn't support everything that 
>> the
>> API allows.
>
> You can easily extend the API to allow multiple backups (In a fully backwards
> compatible way). So there is no need to change that now.

I disagree.

You propose something like:

1.  -> { "execute": "backup",
         "arguments": { "backup-file": "foo.vma" } }
    <- { "return": { "deb8e5da-5b69-46d1-86c6-1b715a22809f" } }

    This *deletes* any prior BackupStatus.  Clearly inappropriate with
    multiple backups.  How exactly do you propose to extend it in a
    backwards compatible way?

2.  -> { "execute": "query-backup" }

    This is specified to return exactly one BackupStatus.

    How exactly do you propose to extend it for multiple backups?

3.  -> { "execute": "backup-cancel" }

    This is specified to cancel "the current execute backup process".

    This becomes *ill-defined* when multiple backup processes are
    executing.

    With multiple backup support, we still need to keep it working
    unchanged when no or just one backup is executing.  When more are
    executing, we have to make it fail, unless an optional argument is
    given.

I don't like the resulting complexity at all.

I'd rather do:

1.  -> { "execute": "backup",
         "arguments": { "id": "client-chosen-id",
                        "backup-file": "foo.vma" } }
    <- { "return": { } }

    Restriction: fails if a backup already exists, regardless of its
    state.  This is intended to be the only bit that needs to be changed
    when we go to multiple backups.

    Note that I have the client pass an id, for consistency with other
    QMP commands.

    I could be persuaded to return an UUID, if you can demonstrate a
    need for it.

2.  -> { "execute": "query-backup" }

    Returns status of all backups, as a list of BackupStatus.  Backups
    stay around until the client gets rid of them (see 4.).

    If you like, you can add suitable optional arguments to get only
    selected status.

3.  -> { "execute": "backup-cancel",
         "arguments": { "id": "client-chosen-id" } }

    If backup is in state "active", make it transition to state "error",
    and succeed.  Else fail.

4.  -> { "execute": "backup-forget",
         "arguments": { "id": "client-chosen-id" } }

    If backup is in state "done" or "error", free remaining resources
    associated with it (id becomes invalid) and succeed.  Else fail.