Re: [Qemu-devel] [PATCH 5/6] snapshot: qmp interface

[Eric Blake]

On 01/03/2013 11:02 PM, Wenchao Xia wrote:

> 
>>> +#                 case, it is the internal snapshot record's name
>>> and if it is
>>> +#                 'blank' name will be generated according to time.
>>
>> Ugg.  Passing an empty string for snapshot-file as a special case seems
>> awkward; it might be better to make it an optional argument via
>> '*snapshot-file', where the argument is mandatory for external, but
>   I think *snapshot-file is great, but it will change the interface
> which already exist triggering compatialbility issue, is this allowed?

Making a formerly-required argument optional is NOT an API break - all
old callers will already be providing the argument, and only new callers
will take advantage of the ability to omit the argument.  So yes, this
change is allowed.  Furthermore, if you properly issue an error in the
cases where a name is required (external snapshots), then marking the
argument optional only affects the cases where it makes sense (internal
snapshots).

>> omitting the argument on internal allows the fallback naming.  Or why do
>> you even need to worry about fallback naming?  Requiring the user to
>> always provide a record name may be easier to support (certainly fewer
>> corner cases to worry about).
>>
>   If cost is low, I think providing it is not bad, and thus hmp/qmp
> can have same capability.

Do we want to support omitting the name in qcow2?  If so, then you have
to provide that capability.  If not, then HMP can still provide the
capability, by generating a name before calling into QMP, while still
making QMP simpler by always requiring a name.  I don't care either way
- when libvirt generates a snapshot, it will always provide a name (as
nameless snapshots are harder to deal with).  Note, however, that even
if we make creation always require a name, we still have to worry about
deletion being able to use a nameless snapshot, thanks to pre-existing
qcow2 files that had no name.  And since there are pre-existing files
with no name, you could argue that taking away the ability to create a
nameless snapshot would be a regression, so maybe you have to support
creation of a nameless snapshot via QMP after all.

>>   Besides, shouldn't it be pretty obvious from the snapshot-file argument
>> whether the snapshot exists internally or externally, without needing
>> the user to tell you that?
>>
>   I thought it is not easy to tell the difference just from file, could
> u tip how?

If a snapshot name is given but no indication of whether it is external
or internal, then the obvious implementation is to iterate through all
internal names, and if any of them match, then it was internal;
otherwise assume it is external.  If it is possible to create both an
internal and an external snapshot using the same name (that is, if
internal names can look like valid file names), then the user would have
to be explicit which one they want; or you could check if an external
file exists with the same name as an internal and error out that a name
is ambiguous if they user was not explicit in that case.


>>> +##
>>> +# @blockdev-snapshot-delete-sync
>>> +#
>>> +# Delete a synchronous snapshot of a block device.
>>
>> Wrong.  Rather, this is a synchronous operation that deletes a snapshot,
>> regardless of whether the snapshot was created synchronously or
>> asynchronously.
>>
>   OK, the doc will be changed to tip better.
> 
>> Remember, the original goal was to come up with a way to take snapshots
>> in a way that didn't require blocking until the operation was done; and
>> once such an interface is present, then that becomes a new operation
>> blockdev-snapshot-async (or more than one command, in order to drive the
>> sequence of operations needed).  blockdev-snapshot-sync would then be
>> rewritten as a wrapper that calls the underlying sequence of operations
>> in one command, blocking until complete.
>>
>> But for deletion, we might as well do it right from the beginning.  I
>> wonder if you can even design things to just have
>> 'blockdev-snapshot-delete' without needing to distinguish between a sync
>> or async operation.
>>
>   There is already API as blockdev-snapshot-sync, this will break
> the mirror, do you think it is OK? Actually I think it is OK
> to redesign API including old one as blockdev-snapshot,

Asymmetric naming is not a problem.  The names should be descriptive of
what they do.  Right now, we only have one command, which makes a
blockdev snapshot, and does so synchronously (blocks for an indefinite
amount of time).  We've left the door open for a new command (or more
likely, a sequence of new commands, one to start, one to abort, and one
to check progress, as well as an event when things are complete) for
doing a snapshot without an arbitrary blocking downtime.  But whether a
snapshot was created synchronously or asynchronously is not recorded in
the snapshot itself, so if deletion can always be done without blocking,
then we don't need two different styles of delete, and a -sync or -async
suffix in the delete command doesn't make any difference.

>  blockdev-snapshot-delete, but again it brings compatible issue.
> Or another solution which keep API unchanged as:
> blockdev-snapshot-sync
> blockdev-snapshot-delete-sync
> blockdev-snapshot-async
> blockdev-snapshot-delete-async

Symmetry in the naming is not important.  Does deleting a snapshot
require an arbitrary length of time?  If so, we should plan for how to
make it async, and track it's progress or abort it early.  If not, then
a single command that always completes fast and does the deletion is
sufficient, and we can name the command just blockdev-snapshot-delete.

I think it would help if you take a step back and give a higher-level
overview of how your commands will be used - what sequence of QMP
commands does a monitor app issue to do various snapshot actions?  Once
that sequence is determined, we can come up with names to fit.

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

signature.asc
Description: OpenPGP digital signature