Patchwork [5/6] snapshot: qmp interface

login
register
mail settings
Submitter Wayne Xia
Date Dec. 17, 2012, 6:25 a.m.
Message ID <1355725509-5429-6-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/206780/
State New
Headers show

Comments

Wayne Xia - Dec. 17, 2012, 6:25 a.m.
This patch changes the implemtion of external block snapshot
to use internal unified interface, now qmp handler just do
a translation of request and submit.
  Also internal block snapshot qmp interface was added.
  Now add external snapshot, add/delete internal snapshot
can be started in their own qmp interface or a group of
BlockAction in qmp transaction interface.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 blockdev.c       |  352 +++++++++++++++++++++++++++++++-----------------------
 qapi-schema.json |  102 ++++++++++++++--
 2 files changed, 292 insertions(+), 162 deletions(-)
Eric Blake - Jan. 2, 2013, 2:52 p.m.
On 12/16/2012 11:25 PM, Wenchao Xia wrote:
>   This patch changes the implemtion of external block snapshot

s/implemtion/implementation/

> to use internal unified interface, now qmp handler just do

s/do/does/

> a translation of request and submit.
>   Also internal block snapshot qmp interface was added.
>   Now add external snapshot, add/delete internal snapshot
> can be started in their own qmp interface or a group of
> BlockAction in qmp transaction interface.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---

> +++ b/qapi-schema.json

I didn't look at the code, because I want to make sure we get the
interface right, first.

> @@ -1458,17 +1458,36 @@
>  { 'command': 'block_resize', 'data': { 'device': 'str', 'size': 'int' }}
>  
>  ##
> +# @SnapshotType
> +#
> +# An enumeration that tells QEMU what type of snapshot to access.
> +#
> +# @internal: QEMU should use internal snapshot in format such as qcow2.
> +#
> +# @external: QEMU should use backing file chain.
> +#
> +# Since: 1.4.
> +##
> +{ 'enum': 'SnapshotType'
> +  'data': [ 'internal', 'external' ] }
> +
> +##
>  # @NewImageMode
>  #
>  # An enumeration that tells QEMU how to set the backing file path in
> -# a new image file.
> +# a new image file, or how to use internal snapshot record.
>  #
> -# @existing: QEMU should look for an existing image file.
> +# @existing: QEMU should look for an existing image file or internal snapshot
> +#            record. In external snapshot case, qemu will skip create new image
> +#            file, In internal snapshot case qemu will try use the existing

s/In/in/

> +#            one. if not found operation would fail.

s/. if/. If/; s/would/will/

>  #
> -# @absolute-paths: QEMU should create a new image with absolute paths
> -# for the backing file.
> +# @absolute-paths: QEMU should create a new image with absolute paths for
> +#                  the backing file in external snapshot case, or create a new
> +#                  snapshot record in internal snapshot case which will
> +#                  overwrite internal snapshot record if it already exist.

Doesn't quite make sense - internal snapshots don't record a path, so
why is absolute-paths the right mode for requesting the creation of a
new snapshot?   I think it would make more sense if you add a new mode,
and then declare that absolute-paths is invalid for internal snapshots,
and that the new mode is invalid for external snapshots.

>  #
> -# Since: 1.1
> +# Since: 1.1, internal support since 1.4.
>  ##
>  { 'enum': 'NewImageMode'
>    'data': [ 'existing', 'absolute-paths' ] }
> @@ -1478,16 +1497,39 @@
>  #
>  # @device:  the name of the device to generate the snapshot from.
>  #
> -# @snapshot-file: the target of the new image. A new file will be created.
> +# @snapshot-file: the target name of the snapshot. In external case, it is
> +#                 the new file's name, A new file will be created. In internal

s/A/a/

and a new file is only created according to mode.

> +#                 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
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).

>  #
>  # @format: #optional the format of the snapshot image, default is 'qcow2'.
>  #
> -# @mode: #optional whether and how QEMU should create a new image, default is
> -#        'absolute-paths'.
> +# @mode: #optional whether QEMU should create a new snapshot or use existing
> +#        one, default is 'absolute-paths'.

Does this default still make sense for internal snapshots, or do you
need to document that the default mode differs depending on the type of
snapshot being taken?

> +#
> +# @type: #optional internal snapshot or external, default is 'external'.

Mention that this field is new since 1.4.

> +#
>  ##
>  { 'type': 'BlockdevSnapshot',
>    'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
> -            '*mode': 'NewImageMode' } }
> +            '*mode': 'NewImageMode', '*type': 'SnapshotType'} }
> +
> +##
> +# @BlockdevSnapshotDelete
> +#
> +# @device:  the name of the device to delete the snapshot from.
> +#
> +# @snapshot-file: the target name of the snapshot. In external case, it is
> +#                 the file's name to be merged, In internal case, it is the
> +#                 internal snapshot record's name.

What happens if there is no record name (since the qcow2 file does not
require one)?

> +#
> +# @type: #optional internal snapshot or external, default is
> +#        'external', note that delete 'external' snapshot is not supported
> +#        now for that it is the same to commit it.

If external is not supported, then why do you even need this parameter?
 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?

> +##
> +{ 'type': 'BlockdevSnapshotDelete',
> +  'data': { 'device': 'str', 'snapshot-file': 'str',
> +            '*type': 'SnapshotType'} }
>  
>  ##
>  # @BlockdevAction
> @@ -1498,6 +1540,7 @@
>  { 'union': 'BlockdevAction',
>    'data': {
>         'blockdev-snapshot-sync': 'BlockdevSnapshot',
> +       'blockdev-snapshot-delete-sync': 'BlockdevSnapshotDelete',
>     } }
>  
>  ##
> @@ -1530,23 +1573,54 @@
>  #
>  # @device:  the name of the device to generate the snapshot from.
>  #
> -# @snapshot-file: the target of the new image. If the file exists, or if it
> -#                 is a device, the snapshot will be created in the existing
> -#                 file/device. If does not exist, a new file will be created.
> +# @snapshot-file: the target name of the snapshot. In external case, it is
> +#                 the new file's name, A new file will be created. If the file
> +#                 exists, or if it is a device, the snapshot will be created in
> +#                 the existing file/device. If does not exist, a new file will
> +#                 be created. In internal case, it is the internal snapshot
> +#                 record's name, if it is 'blank' name will be generated
> +#                 according to time.

Again, special casing the empty string seems awkward; making the field
optional for the internal case might make more sense.

>  #
>  # @format: #optional the format of the snapshot image, default is 'qcow2'.

Is this field compatible with internal snapshots?

>  #
>  # @mode: #optional whether and how QEMU should create a new image, default is
>  #        'absolute-paths'.
>  #
> +# @type: #optional internal snapshot or external, default is
> +#        'external'.

Mention that this field was added in 1.4.

> +#
>  # Returns: nothing on success
>  #          If @device is not a valid block device, DeviceNotFound
>  #
> -# Since 0.14.0
> +# Since 0.14.0, internal snapshot supprt since 1.4.
>  ##
>  { 'command': 'blockdev-snapshot-sync',
>    'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
> -            '*mode': 'NewImageMode'} }
> +            '*mode': 'NewImageMode', '*type': 'SnapshotType'} }
> +
> +##
> +# @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.

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.

> +#
> +# @device:  the name of the device to delete the snapshot from.
> +#
> +# @snapshot-file: the target name of the snapshot. In external case, it is
> +#                 the file's name to be merged, In internal case, it is the
> +#                 internal snapshot record's name.
> +#
> +# @type: #optional internal snapshot or external, default is
> +#        'external', note that delete 'external' snapshot is not supported
> +#        now for that it is the same to commit it.

Again, do you really need this field, or can it be inferred from
snapshot-file?

> +#
> +# Returns: nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#
> +# Since 1.4
> +##
> +{ 'command': 'blockdev-snapshot-delete-sync',
> +  'data': { 'device': 'str', 'snapshot-file': 'str',
> +            '*type': 'SnapshotType'} }
>  
>  ##
>  # @human-monitor-command:
>
Wayne Xia - Jan. 4, 2013, 6:02 a.m.
Thanks for reviewing, I'll correct the spelling.

>> +
>> +##
>>   # @NewImageMode
>>   #
>>   # An enumeration that tells QEMU how to set the backing file path in
>> -# a new image file.
>> +# a new image file, or how to use internal snapshot record.
>>   #
>> -# @existing: QEMU should look for an existing image file.
>> +# @existing: QEMU should look for an existing image file or internal snapshot
>> +#            record. In external snapshot case, qemu will skip create new image
>> +#            file, In internal snapshot case qemu will try use the existing
>
> s/In/in/
>
>> +#            one. if not found operation would fail.
>
> s/. if/. If/; s/would/will/
>
>>   #
>> -# @absolute-paths: QEMU should create a new image with absolute paths
>> -# for the backing file.
>> +# @absolute-paths: QEMU should create a new image with absolute paths for
>> +#                  the backing file in external snapshot case, or create a new
>> +#                  snapshot record in internal snapshot case which will
>> +#                  overwrite internal snapshot record if it already exist.
>
> Doesn't quite make sense - internal snapshots don't record a path, so
> why is absolute-paths the right mode for requesting the creation of a
> new snapshot?   I think it would make more sense if you add a new mode,
> and then declare that absolute-paths is invalid for internal snapshots,
> and that the new mode is invalid for external snapshots.
>
   OK,

>>   #
>> -# Since: 1.1
>> +# Since: 1.1, internal support since 1.4.
>>   ##
>>   { 'enum': 'NewImageMode'
>>     'data': [ 'existing', 'absolute-paths' ] }
>> @@ -1478,16 +1497,39 @@
>>   #
>>   # @device:  the name of the device to generate the snapshot from.
>>   #
>> -# @snapshot-file: the target of the new image. A new file will be created.
>> +# @snapshot-file: the target name of the snapshot. In external case, it is
>> +#                 the new file's name, A new file will be created. In internal
>
> s/A/a/
>
> and a new file is only created according to mode.
>
   OK.

>> +#                 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?

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

>>   #
>>   # @format: #optional the format of the snapshot image, default is 'qcow2'.
>>   #
>> -# @mode: #optional whether and how QEMU should create a new image, default is
>> -#        'absolute-paths'.
>> +# @mode: #optional whether QEMU should create a new snapshot or use existing
>> +#        one, default is 'absolute-paths'.
>
> Does this default still make sense for internal snapshots, or do you
> need to document that the default mode differs depending on the type of
> snapshot being taken?
>
   I'll add another optional argument and keeps mode only for external
snapshot.

>> +#
>> +# @type: #optional internal snapshot or external, default is 'external'.
>
> Mention that this field is new since 1.4.
>
   OK.

>> +#
>>   ##
>>   { 'type': 'BlockdevSnapshot',
>>     'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
>> -            '*mode': 'NewImageMode' } }
>> +            '*mode': 'NewImageMode', '*type': 'SnapshotType'} }
>> +
>> +##
>> +# @BlockdevSnapshotDelete
>> +#
>> +# @device:  the name of the device to delete the snapshot from.
>> +#
>> +# @snapshot-file: the target name of the snapshot. In external case, it is
>> +#                 the file's name to be merged, In internal case, it is the
>> +#                 internal snapshot record's name.
>
> What happens if there is no record name (since the qcow2 file does not
> require one)?
   I think snapshot id should be provided then.

>> +#
>> +# @type: #optional internal snapshot or external, default is
>> +#        'external', note that delete 'external' snapshot is not supported
>> +#        now for that it is the same to commit it.
>
> If external is not supported, then why do you even need this parameter?
>   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?

>> +##
>> +{ 'type': 'BlockdevSnapshotDelete',
>> +  'data': { 'device': 'str', 'snapshot-file': 'str',
>> +            '*type': 'SnapshotType'} }
>>
>>   ##
>>   # @BlockdevAction
>> @@ -1498,6 +1540,7 @@
>>   { 'union': 'BlockdevAction',
>>     'data': {
>>          'blockdev-snapshot-sync': 'BlockdevSnapshot',
>> +       'blockdev-snapshot-delete-sync': 'BlockdevSnapshotDelete',
>>      } }
>>
>>   ##
>> @@ -1530,23 +1573,54 @@
>>   #
>>   # @device:  the name of the device to generate the snapshot from.
>>   #
>> -# @snapshot-file: the target of the new image. If the file exists, or if it
>> -#                 is a device, the snapshot will be created in the existing
>> -#                 file/device. If does not exist, a new file will be created.
>> +# @snapshot-file: the target name of the snapshot. In external case, it is
>> +#                 the new file's name, A new file will be created. If the file
>> +#                 exists, or if it is a device, the snapshot will be created in
>> +#                 the existing file/device. If does not exist, a new file will
>> +#                 be created. In internal case, it is the internal snapshot
>> +#                 record's name, if it is 'blank' name will be generated
>> +#                 according to time.
>
> Again, special casing the empty string seems awkward; making the field
> optional for the internal case might make more sense.
>
   OK, if API compatible issue is acceptable.


>>   #
>>   # @format: #optional the format of the snapshot image, default is 'qcow2'.
>
> Is this field compatible with internal snapshots?
>
   no, I'll doc it.

>>   #
>>   # @mode: #optional whether and how QEMU should create a new image, default is
>>   #        'absolute-paths'.
>>   #
>> +# @type: #optional internal snapshot or external, default is
>> +#        'external'.
>
> Mention that this field was added in 1.4.
>
   OK.

>> +#
>>   # Returns: nothing on success
>>   #          If @device is not a valid block device, DeviceNotFound
>>   #
>> -# Since 0.14.0
>> +# Since 0.14.0, internal snapshot supprt since 1.4.
>>   ##
>>   { 'command': 'blockdev-snapshot-sync',
>>     'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
>> -            '*mode': 'NewImageMode'} }
>> +            '*mode': 'NewImageMode', '*type': 'SnapshotType'} }
>> +
>> +##
>> +# @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,
  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

>> +#
>> +# @device:  the name of the device to delete the snapshot from.
>> +#
>> +# @snapshot-file: the target name of the snapshot. In external case, it is
>> +#                 the file's name to be merged, In internal case, it is the
>> +#                 internal snapshot record's name.
>> +#
>> +# @type: #optional internal snapshot or external, default is
>> +#        'external', note that delete 'external' snapshot is not supported
>> +#        now for that it is the same to commit it.
>
> Again, do you really need this field, or can it be inferred from
> snapshot-file?
>
>> +#
>> +# Returns: nothing on success
>> +#          If @device is not a valid block device, DeviceNotFound
>> +#
>> +# Since 1.4
>> +##
>> +{ 'command': 'blockdev-snapshot-delete-sync',
>> +  'data': { 'device': 'str', 'snapshot-file': 'str',
>> +            '*type': 'SnapshotType'} }
>>
>>   ##
>>   # @human-monitor-command:
>>
>
Eric Blake - Jan. 4, 2013, 1:57 p.m.
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.
Stefan Hajnoczi - Jan. 4, 2013, 4:22 p.m.
On Mon, Dec 17, 2012 at 02:25:08PM +0800, Wenchao Xia wrote:
> @@ -1478,16 +1497,39 @@
>  #
>  # @device:  the name of the device to generate the snapshot from.
>  #
> -# @snapshot-file: the target of the new image. A new file will be created.
> +# @snapshot-file: the target name of the snapshot. In external case, it is
> +#                 the new file's name, A new file will be created. In internal
> +#                 case, it is the internal snapshot record's name and if it is
> +#                 'blank' name will be generated according to time.
>  #
>  # @format: #optional the format of the snapshot image, default is 'qcow2'.
>  #
> -# @mode: #optional whether and how QEMU should create a new image, default is
> -#        'absolute-paths'.
> +# @mode: #optional whether QEMU should create a new snapshot or use existing
> +#        one, default is 'absolute-paths'.
> +#
> +# @type: #optional internal snapshot or external, default is 'external'.
> +#
>  ##
>  { 'type': 'BlockdevSnapshot',
>    'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
> -            '*mode': 'NewImageMode' } }
> +            '*mode': 'NewImageMode', '*type': 'SnapshotType'} }

It feels cleaner to define a new BlockdevInternalSnapshot type instead
of overloading the field semantics.  The naming doesn't fit for internal
snapshots (e.g. "snapshot-file", "absolute-paths", etc).

Treat internal snapshots as a new transaction action.  That way we can
cleanly add internal snapshot specific fields in the future.  The
documentation will be clearer because users won't have to consider the
"if type is 'external', then ..., otherwise ...".

Stefan
Wayne Xia - Jan. 5, 2013, 8:38 a.m.
于 2013-1-5 0:22, Stefan Hajnoczi 写道:
> On Mon, Dec 17, 2012 at 02:25:08PM +0800, Wenchao Xia wrote:
>> @@ -1478,16 +1497,39 @@
>>   #
>>   # @device:  the name of the device to generate the snapshot from.
>>   #
>> -# @snapshot-file: the target of the new image. A new file will be created.
>> +# @snapshot-file: the target name of the snapshot. In external case, it is
>> +#                 the new file's name, A new file will be created. In internal
>> +#                 case, it is the internal snapshot record's name and if it is
>> +#                 'blank' name will be generated according to time.
>>   #
>>   # @format: #optional the format of the snapshot image, default is 'qcow2'.
>>   #
>> -# @mode: #optional whether and how QEMU should create a new image, default is
>> -#        'absolute-paths'.
>> +# @mode: #optional whether QEMU should create a new snapshot or use existing
>> +#        one, default is 'absolute-paths'.
>> +#
>> +# @type: #optional internal snapshot or external, default is 'external'.
>> +#
>>   ##
>>   { 'type': 'BlockdevSnapshot',
>>     'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
>> -            '*mode': 'NewImageMode' } }
>> +            '*mode': 'NewImageMode', '*type': 'SnapshotType'} }
>
> It feels cleaner to define a new BlockdevInternalSnapshot type instead
> of overloading the field semantics.  The naming doesn't fit for internal
> snapshots (e.g. "snapshot-file", "absolute-paths", etc).
>
> Treat internal snapshots as a new transaction action.  That way we can
> cleanly add internal snapshot specific fields in the future.  The
> documentation will be clearer because users won't have to consider the
> "if type is 'external', then ..., otherwise ...".
>
> Stefan
>
   After struggling about the parameter meaning, I guess this is what
make things better.

Patch

diff --git a/blockdev.c b/blockdev.c
index 1c38c67..d1bbe23 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1175,6 +1175,195 @@  delete_and_fail:
     return -1;
 }
 
+/* translation from qmp commands */
+static int fill_blk_trsact_create_sync(BlockdevSnapshot *create_sync,
+                                       BlkTransactionStatesSync *st_sync,
+                                       SNTime *time,
+                                       const char *time_str,
+                                       Error **errp)
+{
+    const char *format = "qcow2";
+    enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
+    enum SnapshotType type = SNAPSHOT_TYPE_EXTERNAL;
+    BlockDriverState *bs;
+
+    const char *device = create_sync->device;
+    const char *name = create_sync->snapshot_file;
+    if (create_sync->has_mode) {
+        mode = create_sync->mode;
+    }
+    if (create_sync->has_format) {
+        format = create_sync->format;
+    }
+    if (create_sync->has_type) {
+        type = create_sync->type;
+    }
+
+    /* find the target bs */
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return -1;
+    }
+
+    switch (type) {
+    case SNAPSHOT_TYPE_INTERNAL:
+        st_sync->type = BLK_SNAPSHOT_INTERNAL;
+        break;
+    case SNAPSHOT_TYPE_EXTERNAL:
+        st_sync->type = BLK_SNAPSHOT_EXTERNAL;
+        break;
+    default:
+        st_sync->type = BLK_SNAPSHOT_NOSUPPORT;
+        error_setg(errp, "Device %s requested invalid snapshot"
+                         " type %d.", device, type);
+        return -1;
+    }
+
+    switch (mode) {
+    case NEW_IMAGE_MODE_EXISTING:
+        st_sync->use_existing = TRUE;
+        break;
+    case NEW_IMAGE_MODE_ABSOLUTE_PATHS:
+        st_sync->use_existing = FALSE;
+        break;
+    default:
+        error_setg(errp, "Device %s requested invalid snapshot"
+                         " mode %d.", device, mode);
+        return -1;
+    }
+
+    if (st_sync->type == BLK_SNAPSHOT_INTERNAL) {
+        /* internal case, if caller need create new one with default string */
+        if (((name == NULL) || (name[0] == '\0')) &&
+           (!st_sync->use_existing)) {
+            st_sync->internal.sn_name = time_str;
+        } else {
+            st_sync->internal.sn_name = name;
+        }
+        st_sync->internal.bs = bs;
+        st_sync->internal.time = *time;
+    } else if (st_sync->type == BLK_SNAPSHOT_EXTERNAL) {
+        st_sync->external.new_image_file = name;
+        st_sync->external.format = format;
+        st_sync->external.old_bs = bs;
+    }
+
+    return 0;
+}
+static int fill_blk_trsact_delete_sync(BlockdevSnapshotDelete *delete_sync,
+                                       BlkTransactionStatesSync *st_sync,
+                                       Error **errp)
+{
+    enum SnapshotType type = SNAPSHOT_TYPE_EXTERNAL;
+    BlockDriverState *bs;
+
+    const char *device = delete_sync->device;
+    const char *name = delete_sync->snapshot_file;
+    if (delete_sync->has_type) {
+        type = delete_sync->type;
+    }
+
+    /* find the target bs */
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return -1;
+    }
+
+    switch (type) {
+    case SNAPSHOT_TYPE_INTERNAL:
+        st_sync->type = BLK_SNAPSHOT_INTERNAL;
+        break;
+    case SNAPSHOT_TYPE_EXTERNAL:
+        st_sync->type = BLK_SNAPSHOT_EXTERNAL;
+        break;
+    default:
+        st_sync->type = BLK_SNAPSHOT_NOSUPPORT;
+        error_setg(errp, "Device %s requested invalid snapshot"
+                         " type %d.", device, type);
+        return -1;
+    }
+
+    if (st_sync->type == BLK_SNAPSHOT_INTERNAL) {
+        st_sync->internal.sn_name = name;
+        st_sync->internal.bs = bs;
+    } else if (st_sync->type == BLK_SNAPSHOT_EXTERNAL) {
+        st_sync->external.new_image_file = name;
+        st_sync->external.old_bs = bs;
+    }
+
+    return 0;
+}
+
+static int fill_blk_trsact(BlockdevAction *dev_info,
+                           BlkTransactionStates *states,
+                           SNTime *time,
+                           const char *time_str,
+                           Error **errp)
+{
+    switch (dev_info->kind) {
+    case BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC:
+        states->st_sync.op = BLK_SN_SYNC_CREATE;
+        states->async = FALSE;
+        return fill_blk_trsact_create_sync(dev_info->blockdev_snapshot_sync,
+                                   &states->st_sync, time, time_str, errp);
+        break;
+    case BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_DELETE_SYNC:
+        states->st_sync.op = BLK_SN_SYNC_DELETE;
+        states->async = FALSE;
+        return fill_blk_trsact_delete_sync(
+                                    dev_info->blockdev_snapshot_delete_sync,
+                                    &states->st_sync, errp);
+        break;
+    default:
+        abort();
+    }
+    return 0;
+}
+
+/* Here this funtion prepare the request list, submit for atomic snapshot. */
+void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
+{
+    BlockdevActionList *dev_entry = dev_list;
+    BlkTransactionStates *states;
+    int ret;
+
+    BlkTransactionStatesList *snap_bdrv_states = blk_trans_st_list_new();
+
+    /* translate qmp request */
+    /* for group snapshot create we use same time stamp here */
+    SNTime time = get_sn_time();
+    char time_str[256];
+    generate_sn_name_from_time(&time, time_str, sizeof(time_str));
+    while (NULL != dev_entry) {
+        BlockdevAction *dev_info = NULL;
+
+        dev_info = dev_entry->value;
+        dev_entry = dev_entry->next;
+
+        states = blk_trans_st_new();
+        ret = fill_blk_trsact(dev_info, states, &time, time_str, errp);
+        if (ret < 0) {
+            blk_trans_st_delete(&states);
+            goto exit;
+        }
+
+        ret = add_transaction(snap_bdrv_states, states, errp);
+        if (ret < 0) {
+            blk_trans_st_delete(&states);
+            goto exit;
+        }
+    }
+
+    /* submit to internal API, no need to check return for no following
+       action now. */
+    submit_transaction(snap_bdrv_states, errp);
+
+exit:
+    blk_trans_st_list_delete(&snap_bdrv_states);
+}
+
 static void blockdev_do_action(int kind, void *data, Error **errp)
 {
     BlockdevAction action;
@@ -1190,6 +1379,7 @@  static void blockdev_do_action(int kind, void *data, Error **errp)
 void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
                                 bool has_format, const char *format,
                                 bool has_mode, enum NewImageMode mode,
+                                bool has_type, enum SnapshotType type,
                                 Error **errp)
 {
     BlockdevSnapshot snapshot = {
@@ -1199,162 +1389,28 @@  void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
         .format = (char *) format,
         .has_mode = has_mode,
         .mode = mode,
+        .has_type = has_type,
+        .type = type,
     };
     blockdev_do_action(BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC, &snapshot,
                        errp);
 }
 
-
-/* New and old BlockDriverState structs for group snapshots */
-typedef struct BlkTransactionStates {
-    BlockDriverState *old_bs;
-    BlockDriverState *new_bs;
-    QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
-} BlkTransactionStates;
-
-/*
- * 'Atomic' group snapshots.  The snapshots are taken as a set, and if any fail
- *  then we do not pivot any of the devices in the group, and abandon the
- *  snapshots
- */
-void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
+void qmp_blockdev_snapshot_delete_sync(const char *device,
+                                       const char *snapshot_file,
+                                       bool has_type, enum SnapshotType type,
+                                       Error **errp)
 {
-    int ret = 0;
-    BlockdevActionList *dev_entry = dev_list;
-    BlkTransactionStates *states, *next;
-    Error *local_err = NULL;
-
-    QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionStates) snap_bdrv_states;
-    QSIMPLEQ_INIT(&snap_bdrv_states);
-
-    /* drain all i/o before any snapshots */
-    bdrv_drain_all();
-
-    /* We don't do anything in this loop that commits us to the snapshot */
-    while (NULL != dev_entry) {
-        BlockdevAction *dev_info = NULL;
-        BlockDriver *proto_drv;
-        BlockDriver *drv;
-        int flags;
-        enum NewImageMode mode;
-        const char *new_image_file;
-        const char *device;
-        const char *format = "qcow2";
-
-        dev_info = dev_entry->value;
-        dev_entry = dev_entry->next;
-
-        states = g_malloc0(sizeof(BlkTransactionStates));
-        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
-
-        switch (dev_info->kind) {
-        case BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC:
-            device = dev_info->blockdev_snapshot_sync->device;
-            if (!dev_info->blockdev_snapshot_sync->has_mode) {
-                dev_info->blockdev_snapshot_sync->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
-            }
-            new_image_file = dev_info->blockdev_snapshot_sync->snapshot_file;
-            if (dev_info->blockdev_snapshot_sync->has_format) {
-                format = dev_info->blockdev_snapshot_sync->format;
-            }
-            mode = dev_info->blockdev_snapshot_sync->mode;
-            break;
-        default:
-            abort();
-        }
-
-        drv = bdrv_find_format(format);
-        if (!drv) {
-            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
-            goto delete_and_fail;
-        }
-
-        states->old_bs = bdrv_find(device);
-        if (!states->old_bs) {
-            error_set(errp, QERR_DEVICE_NOT_FOUND, device);
-            goto delete_and_fail;
-        }
-
-        if (!bdrv_is_inserted(states->old_bs)) {
-            error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
-            goto delete_and_fail;
-        }
-
-        if (bdrv_in_use(states->old_bs)) {
-            error_set(errp, QERR_DEVICE_IN_USE, device);
-            goto delete_and_fail;
-        }
-
-        if (!bdrv_is_read_only(states->old_bs)) {
-            if (bdrv_flush(states->old_bs)) {
-                error_set(errp, QERR_IO_ERROR);
-                goto delete_and_fail;
-            }
-        }
-
-        flags = states->old_bs->open_flags;
-
-        proto_drv = bdrv_find_protocol(new_image_file);
-        if (!proto_drv) {
-            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
-            goto delete_and_fail;
-        }
-
-        /* create new image w/backing file */
-        if (mode != NEW_IMAGE_MODE_EXISTING) {
-            bdrv_img_create(new_image_file, format,
-                            states->old_bs->filename,
-                            states->old_bs->drv->format_name,
-                            NULL, -1, flags, &local_err);
-            if (error_is_set(&local_err)) {
-                error_propagate(errp, local_err);
-                goto delete_and_fail;
-            }
-        }
-
-        /* We will manually add the backing_hd field to the bs later */
-        states->new_bs = bdrv_new("");
-        ret = bdrv_open(states->new_bs, new_image_file,
-                        flags | BDRV_O_NO_BACKING, drv);
-        if (ret != 0) {
-            error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
-            goto delete_and_fail;
-        }
-    }
-
-
-    /* Now we are going to do the actual pivot.  Everything up to this point
-     * is reversible, but we are committed at this point */
-    QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
-        /* This removes our old bs from the bdrv_states, and adds the new bs */
-        bdrv_append(states->new_bs, states->old_bs);
-        /* We don't need (or want) to use the transactional
-         * bdrv_reopen_multiple() across all the entries at once, because we
-         * don't want to abort all of them if one of them fails the reopen */
-        bdrv_reopen(states->new_bs, states->new_bs->open_flags & ~BDRV_O_RDWR,
-                    NULL);
-    }
-
-    /* success */
-    goto exit;
-
-delete_and_fail:
-    /*
-    * failure, and it is all-or-none; abandon each new bs, and keep using
-    * the original bs for all images
-    */
-    QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
-        if (states->new_bs) {
-             bdrv_delete(states->new_bs);
-        }
-    }
-exit:
-    QSIMPLEQ_FOREACH_SAFE(states, &snap_bdrv_states, entry, next) {
-        g_free(states);
-    }
+    BlockdevSnapshotDelete snapshot_delete = {
+        .device = (char *) device,
+        .snapshot_file = (char *) snapshot_file,
+        .has_type = has_type,
+        .type = type,
+    };
+    blockdev_do_action(BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_DELETE_SYNC,
+                       &snapshot_delete, errp);
 }
 
-
 static void eject_device(BlockDriverState *bs, int force, Error **errp)
 {
     if (bdrv_in_use(bs)) {
diff --git a/qapi-schema.json b/qapi-schema.json
index 5dfa052..46f4c6b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1458,17 +1458,36 @@ 
 { 'command': 'block_resize', 'data': { 'device': 'str', 'size': 'int' }}
 
 ##
+# @SnapshotType
+#
+# An enumeration that tells QEMU what type of snapshot to access.
+#
+# @internal: QEMU should use internal snapshot in format such as qcow2.
+#
+# @external: QEMU should use backing file chain.
+#
+# Since: 1.4.
+##
+{ 'enum': 'SnapshotType'
+  'data': [ 'internal', 'external' ] }
+
+##
 # @NewImageMode
 #
 # An enumeration that tells QEMU how to set the backing file path in
-# a new image file.
+# a new image file, or how to use internal snapshot record.
 #
-# @existing: QEMU should look for an existing image file.
+# @existing: QEMU should look for an existing image file or internal snapshot
+#            record. In external snapshot case, qemu will skip create new image
+#            file, In internal snapshot case qemu will try use the existing
+#            one. if not found operation would fail.
 #
-# @absolute-paths: QEMU should create a new image with absolute paths
-# for the backing file.
+# @absolute-paths: QEMU should create a new image with absolute paths for
+#                  the backing file in external snapshot case, or create a new
+#                  snapshot record in internal snapshot case which will
+#                  overwrite internal snapshot record if it already exist.
 #
-# Since: 1.1
+# Since: 1.1, internal support since 1.4.
 ##
 { 'enum': 'NewImageMode'
   'data': [ 'existing', 'absolute-paths' ] }
@@ -1478,16 +1497,39 @@ 
 #
 # @device:  the name of the device to generate the snapshot from.
 #
-# @snapshot-file: the target of the new image. A new file will be created.
+# @snapshot-file: the target name of the snapshot. In external case, it is
+#                 the new file's name, A new file will be created. In internal
+#                 case, it is the internal snapshot record's name and if it is
+#                 'blank' name will be generated according to time.
 #
 # @format: #optional the format of the snapshot image, default is 'qcow2'.
 #
-# @mode: #optional whether and how QEMU should create a new image, default is
-#        'absolute-paths'.
+# @mode: #optional whether QEMU should create a new snapshot or use existing
+#        one, default is 'absolute-paths'.
+#
+# @type: #optional internal snapshot or external, default is 'external'.
+#
 ##
 { 'type': 'BlockdevSnapshot',
   'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
-            '*mode': 'NewImageMode' } }
+            '*mode': 'NewImageMode', '*type': 'SnapshotType'} }
+
+##
+# @BlockdevSnapshotDelete
+#
+# @device:  the name of the device to delete the snapshot from.
+#
+# @snapshot-file: the target name of the snapshot. In external case, it is
+#                 the file's name to be merged, In internal case, it is the
+#                 internal snapshot record's name.
+#
+# @type: #optional internal snapshot or external, default is
+#        'external', note that delete 'external' snapshot is not supported
+#        now for that it is the same to commit it.
+##
+{ 'type': 'BlockdevSnapshotDelete',
+  'data': { 'device': 'str', 'snapshot-file': 'str',
+            '*type': 'SnapshotType'} }
 
 ##
 # @BlockdevAction
@@ -1498,6 +1540,7 @@ 
 { 'union': 'BlockdevAction',
   'data': {
        'blockdev-snapshot-sync': 'BlockdevSnapshot',
+       'blockdev-snapshot-delete-sync': 'BlockdevSnapshotDelete',
    } }
 
 ##
@@ -1530,23 +1573,54 @@ 
 #
 # @device:  the name of the device to generate the snapshot from.
 #
-# @snapshot-file: the target of the new image. If the file exists, or if it
-#                 is a device, the snapshot will be created in the existing
-#                 file/device. If does not exist, a new file will be created.
+# @snapshot-file: the target name of the snapshot. In external case, it is
+#                 the new file's name, A new file will be created. If the file
+#                 exists, or if it is a device, the snapshot will be created in
+#                 the existing file/device. If does not exist, a new file will
+#                 be created. In internal case, it is the internal snapshot
+#                 record's name, if it is 'blank' name will be generated
+#                 according to time.
 #
 # @format: #optional the format of the snapshot image, default is 'qcow2'.
 #
 # @mode: #optional whether and how QEMU should create a new image, default is
 #        'absolute-paths'.
 #
+# @type: #optional internal snapshot or external, default is
+#        'external'.
+#
 # Returns: nothing on success
 #          If @device is not a valid block device, DeviceNotFound
 #
-# Since 0.14.0
+# Since 0.14.0, internal snapshot supprt since 1.4.
 ##
 { 'command': 'blockdev-snapshot-sync',
   'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
-            '*mode': 'NewImageMode'} }
+            '*mode': 'NewImageMode', '*type': 'SnapshotType'} }
+
+##
+# @blockdev-snapshot-delete-sync
+#
+# Delete a synchronous snapshot of a block device.
+#
+# @device:  the name of the device to delete the snapshot from.
+#
+# @snapshot-file: the target name of the snapshot. In external case, it is
+#                 the file's name to be merged, In internal case, it is the
+#                 internal snapshot record's name.
+#
+# @type: #optional internal snapshot or external, default is
+#        'external', note that delete 'external' snapshot is not supported
+#        now for that it is the same to commit it.
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#
+# Since 1.4
+##
+{ 'command': 'blockdev-snapshot-delete-sync',
+  'data': { 'device': 'str', 'snapshot-file': 'str',
+            '*type': 'SnapshotType'} }
 
 ##
 # @human-monitor-command: