Re: [Qemu-devel] [PATCH v4 07/11] qapi: Convert savevm

[Eric Blake]

On 04/09/2013 11:23 AM, Markus Armbruster wrote:
>> If we mandate that a tag always be supplied, then we cannot create qcow2
>> files with a snapshot that lacks a tag using just QMP; but even if we do
> 
> Are you sure you can do that with the proposed patches?  If I read them
> correctly, we make up a tag vm-%Y%m%d%H%M%S when none is given.

HMP is higher-level than QMP, so having HMP default to always generating
a tag seems like a reasonable thing.  In my opinion, it's okay for
higher layers to ensure a name even when lower layers leave it optional.
 Besides, HMP aims to ease user interface and it is easier to work with
named snapshots than it is to work with anonymous ones, even if that
means HMP generates a name.  We already have instances where HMP is
intentionally less powerful than QMP, because if you are using the human
interface, you don't need to be bothered by the subtleties - and if you
need the full power, then you use the lower-level interface.  On the
other hand, we could always change our mind later to have HMP create
anonymous snapshots (by adding another flag to HMP 'savevm') instead of
generating a name.

Where we would get into trouble is if QMP enforces a name but we change
our mind and later want HMP to again expose the possibility of anonymous
snapshots.  Also, if QMP doesn't expose the full power of the file
format on creation, but still has to support the full power of the file
format on the loadvm side, it becomes that much harder to test that the
loadvm support is correct in the presence of anonymous snapshots.

> 
>> that, we STILL have to support use of existing qcow2 files that were
>> created by earlier qemu and/or other tools that have snapshots without a
>> tag.
> 
> I understand the need to deal with existing qcow2 files lacking tags.
> 
> I understand the desire to be able to create such files via QMP.  I
> don't share it, though :)

I'm on the fence - I could live with the requirement that QMP requires a
name, and that creation of anonymous snapshots requires other tools.
Libvirt will always pass a name, for what it is worth, regardless of
whether HMP generates a name, and regardless of whether QMP requires a
name.  But testing nameless support is harder if it requires an extra
tool to create a nameless snapshot in the first place.

> 
> For block driver method bdrv_snapshot_create() both tag and ID are
> optional.  If ID is missing, it picks one.  qcow2 picks max. existing ID
> + 1.  If tag is missing, the snapshot doesn't get one.
> 
> When savevm overwrites an existing snapshot, it reuses both tag and ID.
> One of them matches @name.
> 
> When savevm creates a new snapshot, @name becomes the tag.  If @name
> isn't given, we make one up.  ID is left to the block driver.
> 
> vm-snapshot-save behaves the same.
> 
> Say we have two snapshots, with IDs 1 and 5.  Say I want to overwrite
> the first one, and chose to identify it by its ID.  Since I got dirt on
> my monitor, I misread ID 7.  Since neither ID nor tag 7 exist, I
> accidentally create a new snapshot with tag 7 and ID 6 (picked by qcow2
> block driver).  Next new snapshot will inevitably get ID 7 and the
> confusion is complete.  All because the stupid command doesn't let me
> express myself clearly: I mean *ID* 7, *not* tag 7!

Yep - another instance of the confusion that current HMP 'savevm' has,
and that I want to get rid of by adding -f as a force option at a
minimum to the HMP layer.

> 
> I think I'd try the following for QMP:
> 
> * Drop the capability to overwrite existing snapshot.  No real loss, as
>   it's not an atomic overwrite: it first deletes the old one, then
>   creates the new one, and if create fails, the old one's still gone.

We'd _still_ want to add HMP 'savevm -f' to force an overwrite, but you
are saying that the HMP command would then be a sequence of _two_ QMP
calls - delete then create, where QMP create _always_ fails if a
snapshot already exists with that name.  Makes sense - it just means
that the 'force' semantics are now put as a burden on the caller,
instead of on QMP.

> 
> * Take parameter @tag and @id.
> 
>   Fail if @tag is given and matches any existing tag.  If it's not
>   given, we make one up that doesn't match any existing tag.

Generating an id makes sense.  And the command must return the id that
got used (whether passed in by the user or generated).

> 
>   Fail if @id is given and matches any existing ID.  If it's not given,
>   we make one up that doesn't match any existing ID.

I don't know if I like the idea of QMP generating a tag, vs. leaving the
snapshot anonymous.  But note that the answer to the next question
determines whether we can bypass this question, since if...

> 
> * Seriously consider making @tag mandatory.

...tag is made mandatory, then QMP never has to generate a tag.  I'm
still on the fence on whether to always have a tag in place (whether by
QMP generation or by mandatory argument) vs. allowing anonymous
snapshots.  But again, libvirt always names its snapshots, and I trust
your judgment as maintainer of this area of code in which way we should go.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature