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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 04/09/2013 10:04 AM, Markus Armbruster wrote:
>
>>> +
>>> +##
>>> +# @vm-snapshot-save:
>>> +#
>>> +# Create a snapshot of the whole virtual machine. If tag is
>>> provided as @name,
>>> +# it is used as human readable identifier. If there is already a snapshot
>>> +# with the same tag or id, the force argument needs to be true to
>>> replace it.
>> 
>> "tag or id"?
>> 
>> HMP command savevm's argument is matched against both QEMUSnapshotInfo
>> member id_str (a.k.a. id) and name (a.k.a. tag).  Do we really want that
>> kind of overloading in QMP?  Shouldn't we make it tag vs. id explicit
>> there?
>
> The qcow2 file format already allows for the creation of a snapshot with
> an id but no tag.  We've been back and forth about whether QMP should
> allow a user to be able to create all valid qcow2 files (and hence allow
> the omission of tag).  But things really get confusing if you allow the

I don't have all that context; I hope I'm not wasting everyone's time.

> creation of a tag that matches an existing id (create a snapshot, then
> create another snapshot with the tag '1'); or even if a tag can match a
> potential future id (create a snapshot with a tag named '2', then create
> another snapshot).  So the lookup should always check for BOTH tag and
> id, even though the command is supplying only a 'tag'; and if a tag is
> omitted, a unique id will always be returned.
>
>> 
>>> +#
>>> +# The VM is automatically stopped and resumed and saving a snapshot can take
>>> +# a long time.
>>> +#
>>> +# @name: #optional tag of new snapshot or tag|id of existing snapshot
>> 
>> Should we make this mandatory?  We have to keep the argument optional in
>> HMP, but that needn't concern us here.
>
> 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.

> 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 :)

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!

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.

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

  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.

* Seriously consider making @tag mandatory.