qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID any


From: Daniel Henrique Barboza
Subject: Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore
Date: Thu, 10 Jan 2019 11:03:09 -0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1



On 1/10/19 9:41 AM, Dr. David Alan Gilbert wrote:
* Kevin Wolf (address@hidden) wrote:
Am 09.01.2019 um 20:02 hat Eric Blake geschrieben:
On 1/9/19 12:51 PM, Kevin Wolf wrote:

Indeed, and libvirt IS using 'savevm' via HMP via QMP's
human-monitor-command, since there is no QMP counterpart for internal
snapshot.  Even though lately we consistently tell people that internal
snapshots are underdeveloped and you should use external snapshots, it
does not get away from the fact that libvirt has been using 'savevm' to
drive internal snapshots for years now, and that we MUST consider
back-compat and/or add an introspectible QMP interface before making
changes that would break libvirt.
Okay, so what does libvirt do when you request a snapshot with a
numerical name? Without having looked at the code, the best case I would
expect that it forbids them, and more realistically I suspect that we
may actually fix a bug for libvirt by changing the semantics.

Or does libvirt really use snapshot IDs rather than names?
At the moment, libvirt does not place any restriction on internal
snapshot names, but passes the user's name through without thought of
whether it is an id or a name.

So yes, arguably tightening the semantics fixes a libvirt bug for
libvirt having allowed internal snapshots to get into an inconsistent
state.
So there are two scenarios to consider with respect to breaking
backwards compatibility:

1. There may be code out there that relies on numeric arguments being
    interpreted as IDs. This code will break if we make this change and
    numeric snapshot names exist. That such code exists is speculation
    (even though plausible) and we don't know how widespread it is.

2. There may be code out there that blindly assumes that whatever it
    passes is interpreted as a name. Nobody considered that with a
    numeric snapshot name, it maybe misinterpreted as an ID. We know that
    this is true for libvirt, the single most used management tool for
    QEMU. More code like this may (and probably does) exist.

Essentially the same two categories exist for human users: those who
somehow found out that QEMU accepts IDs as monitor command arguments and
are using those (mitigated by not displaying IDs any more), and those
who are trapped because they wanted to access a numeric name, but
surprisingly got it interpreted as an ID. Both are speculation to some
degree, but my guess is that the latter group is larger.

Given all this, this is a no-brainer for me. We simplify the interface,
we simplify the code, we make things less confusing for manual users and
we fix the management tool that everybody uses. How could we not make
this change?

But again, it falls in the category of "properly fixing this
requires a lot of auditing, time, mental power, and unit tests", which
makes it a rather daunting task to state for certain.
Fix the world before we can fix anything?
Can't we break this loop for the savevm command fairly easily; it's
currently documnted as:

savevm [tag|id]

If we made that:

savevm [-t] [-i] [tag|id]

then:
   a) with neither -t or -i  it would behave in the same roulette way
      as it does in the moment, and it might be a tag or id

The roulette we have today is too confusing IMO. For starters, we're not creating
snapshot using ID at this moment. The ID is being auto-generated by doing
max_ID_found + 1.

Quick example: savevm 4 isn't creating a snapshot with ID=4 and an empty tag - it is creating a snapshot with ID=ID_MAX + 1 and tag = 4. Let's say that ID_MAX was 9,
so this new snapshot would be ID=10, tag=4.

Now, the user wants to delete this snapshot. "delvm 4" will  first match ID, then tag. If there is a snapshot with ID=4, it will be mistakenly erased. Note that this isn't
a bug - even with tweaks here and there, savevm/delvm (number) will always
be ambiguous because the API is allowing the user to interpret an input anyway
he/she wants. This is an API design issue.

The only way I see to reliably make this id|tag API to work is to always interpret numerical arguments as ID. Then we can safely assume what the user wants to do. We wouldn't even need the other 2 options you mentioned (-t and -i). But then, as I've said somewhere in this thread, users would complain that "savevm 4" isn't doing what it was doing before. "savevm 4 is creating snapshots with auto-generated
tag instead of  tag=4". I can see the bugzillas already.



DHB




   b) with -t we'd explicitly treat the parameter as a tag and it
      would error if it wasn't found

   c) With -i we'd explicitly treat the parameter as an id and
      it would error if it wasn't found

Since we still allow (a) it doesn't break any existing code.

Dave


Kevin

--
Dr. David Alan Gilbert / address@hidden / Manchester, UK




reply via email to

[Prev in Thread] Current Thread [Next in Thread]