Patchwork [V2,08/12] qmp: add interface blockdev-snapshot-internal-sync

login
register
mail settings
Submitter Wayne Xia
Date June 14, 2013, 11:39 a.m.
Message ID <1371209999-15579-9-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/251388/
State New
Headers show

Comments

Wayne Xia - June 14, 2013, 11:39 a.m.
This interface can generate snapshot name automatically if it is not
specified, since it is a single opertion.

Snapshot ID can't be specified in this interface.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 blockdev.c       |   25 +++++++++++++++++++++++++
 qapi-schema.json |   23 +++++++++++++++++++++++
 qmp-commands.hx  |   31 +++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+), 0 deletions(-)
Eric Blake - June 15, 2013, 9:51 a.m.
On 06/14/2013 12:39 PM, Wenchao Xia wrote:
> This interface can generate snapshot name automatically if it is not
> specified, since it is a single opertion.

s/opertion/operation/

> 
> Snapshot ID can't be specified in this interface.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  blockdev.c       |   25 +++++++++++++++++++++++++
>  qapi-schema.json |   23 +++++++++++++++++++++++
>  qmp-commands.hx  |   31 +++++++++++++++++++++++++++++++
>  3 files changed, 79 insertions(+), 0 deletions(-)
> 

> +++ b/qapi-schema.json
> @@ -1689,6 +1689,29 @@
>              '*mode': 'NewImageMode'} }
>  
>  ##
> +# @blockdev-snapshot-internal-sync
> +#
> +# Synchronously take an internal snapshot of a block device, when the format
> +# of the image used supports it.
> +#
> +# @device: the name of the device to generate the snapshot from
> +#
> +# @name: #optional the new snapshot name. If not specified, a name will be
> +# generated according to time by qemu

So why is name optional here but mandatory within a transaction?  If
qemu is able to generate names, then it should be able to generate names
in both cases.  Otherwise, make the name mandatory in both places.

Should this patch be folded in to 7/12?  Compare with Stefan's series on
adding block-snapshot as a transaction (for that matter, the two series
have a [trivial] merge conflict since both add a transaction), and make
sure you are using the same approach between the two series at
introducing things.

> +#
> +# Returns: nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#          If any snapshot matching @name exists, or the name is a numeric
> +#          which may mess up with snapshot ID, generic error will be returned
> +#          If the format of the image used does not support it,
> +#          BlockFormatFeatureNotSupported
> +#
> +# Since 1.6
> +##
> +{ 'command': 'blockdev-snapshot-internal-sync',
> +  'data': { 'device': 'str', '*name': 'str'} }

Ah, so this answers my question in 7/12 about a witness that libvirt can
use for knowing when transaction supports the new action, without
needing introspection.


> +
> +SQMP
> +blockdev-snapshot-internal-sync
> +-------------------------------
> +
> +Synchronously take an internal snapshot of a block device when the format of
> +image used supports it.  If name is not specified, it will be automatically
> +generated by qemu according to host time.  If the name is a numeric string
> +which may mess up with ID, such as "19", the operation will fail.

Wait a second.  If we DON'T pass a name, then the generated name is all
numeric.  But if we DO pass an all-numeric name, it gets rejected.
That's awkward to explain.  Maybe you want to instead have a cutoff,
where a number < 64k (is that the right threshold? I just picked a
number out of the air) is rejected, but a number >= 64k is treated as
valid because it might represent a timestamp.  Or allow all possible
numbers, and only reject the creation of a name that collides with an
existing id.  It may be too hard to predict if a name will collide with
a future id of a later snapshot operation.


> +Example:
> +
> +-> { "execute": "blockdev-snapshot-internal-sync",
> +                "arguments": { "device": "ide-hd0",
> +                               "name": "snapshot0" }
> +   }
> +<- { "return": {} }

Evil.  If I don't pass a name, then I NEED to know what name got
generated on my behalf.  So that argues you need to return something,
rather than nothing.  I can see why you can't return a string via
'transaction', but maybe this is an argument that 'name' should be
mandatory in this QMP command (and any generation of a timestamp id must
be higher up in the stack, at the HMP level, so that HMP can still treat
name as optional).  But then you are back to solving the problem of
allowing an all-numeric generated timestamp as a valid name.
Wayne Xia - June 17, 2013, 3:09 a.m.
于 2013-6-15 17:51, Eric Blake 写道:
> On 06/14/2013 12:39 PM, Wenchao Xia wrote:
>> This interface can generate snapshot name automatically if it is not
>> specified, since it is a single opertion.
>
> s/opertion/operation/
>
>>
>> Snapshot ID can't be specified in this interface.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   blockdev.c       |   25 +++++++++++++++++++++++++
>>   qapi-schema.json |   23 +++++++++++++++++++++++
>>   qmp-commands.hx  |   31 +++++++++++++++++++++++++++++++
>>   3 files changed, 79 insertions(+), 0 deletions(-)
>>
>
>> +++ b/qapi-schema.json
>> @@ -1689,6 +1689,29 @@
>>               '*mode': 'NewImageMode'} }
>>
>>   ##
>> +# @blockdev-snapshot-internal-sync
>> +#
>> +# Synchronously take an internal snapshot of a block device, when the format
>> +# of the image used supports it.
>> +#
>> +# @device: the name of the device to generate the snapshot from
>> +#
>> +# @name: #optional the new snapshot name. If not specified, a name will be
>> +# generated according to time by qemu
>
> So why is name optional here but mandatory within a transaction?  If
> qemu is able to generate names, then it should be able to generate names
> in both cases.  Otherwise, make the name mandatory in both places.
   A bit different: transaction take multiple requests, if some thing
is generated it should return the info, but it didn't, so forbid it.
But for this action, it is single, make caller possible to get accurate
info by a info query. I'll make name mandatory in both case, it seems
better.


>
> Should this patch be folded in to 7/12?  Compare with Stefan's series on
   I think the two patch can distinguish each other: one is doing the job
in "batch" mode, one is doing in "single" mode, and make each patch
smaller. After review, I am OK to squash them.

> adding block-snapshot as a transaction (for that matter, the two series
> have a [trivial] merge conflict since both add a transaction), and make
> sure you are using the same approach between the two series at
> introducing things.
   I am OK to rebase if Stefan's patch upstream first.

>
>> +#
>> +# Returns: nothing on success
>> +#          If @device is not a valid block device, DeviceNotFound
>> +#          If any snapshot matching @name exists, or the name is a numeric
>> +#          which may mess up with snapshot ID, generic error will be returned
>> +#          If the format of the image used does not support it,
>> +#          BlockFormatFeatureNotSupported
>> +#
>> +# Since 1.6
>> +##
>> +{ 'command': 'blockdev-snapshot-internal-sync',
>> +  'data': { 'device': 'str', '*name': 'str'} }
>
> Ah, so this answers my question in 7/12 about a witness that libvirt can
> use for knowing when transaction supports the new action, without
> needing introspection.
>
>
>> +
>> +SQMP
>> +blockdev-snapshot-internal-sync
>> +-------------------------------
>> +
>> +Synchronously take an internal snapshot of a block device when the format of
>> +image used supports it.  If name is not specified, it will be automatically
>> +generated by qemu according to host time.  If the name is a numeric string
>> +which may mess up with ID, such as "19", the operation will fail.
>
> Wait a second.  If we DON'T pass a name, then the generated name is all
   Nop, it starts with "vm", like "vm-20130608141726". But I'll make
parameter name mandatory in next version.

> numeric.  But if we DO pass an all-numeric name, it gets rejected.
> That's awkward to explain.  Maybe you want to instead have a cutoff,
> where a number < 64k (is that the right threshold? I just picked a
> number out of the air) is rejected, but a number >= 64k is treated as
> valid because it might represent a timestamp.  Or allow all possible
> numbers, and only reject the creation of a name that collides with an
> existing id.  It may be too hard to predict if a name will collide with
> a future id of a later snapshot operation.
>
>
>> +Example:
>> +
>> +-> { "execute": "blockdev-snapshot-internal-sync",
>> +                "arguments": { "device": "ide-hd0",
>> +                               "name": "snapshot0" }
>> +   }
>> +<- { "return": {} }
>
> Evil.  If I don't pass a name, then I NEED to know what name got
> generated on my behalf.  So that argues you need to return something,
> rather than nothing.  I can see why you can't return a string via
> 'transaction', but maybe this is an argument that 'name' should be
> mandatory in this QMP command (and any generation of a timestamp id must
> be higher up in the stack, at the HMP level, so that HMP can still treat
> name as optional).  But then you are back to solving the problem of
> allowing an all-numeric generated timestamp as a valid name.
>

Patch

diff --git a/blockdev.c b/blockdev.c
index aaeb0e8..6a952cd 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -777,6 +777,31 @@  void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
                        &snapshot, errp);
 }
 
+void qmp_blockdev_snapshot_internal_sync(const char *device,
+                                         bool has_name, const char *name,
+                                         Error **errp)
+{
+    qemu_timeval tv;
+    struct tm tm;
+    char name1[128];
+
+    BlockdevSnapshotInternal snapshot = {
+        .device = (char *) device,
+    };
+
+    if (has_name) {
+        snapshot.name = (char *) name;
+    } else {
+        qemu_gettimeofday(&tv);
+        localtime_r((const time_t *)&tv.tv_sec, &tm);
+        strftime(name1, sizeof(name1), "vm-%Y%m%d%H%M%S", &tm);
+        snapshot.name = name1;
+    }
+
+    blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC,
+                       &snapshot, errp);
+}
+
 
 /* New and old BlockDriverState structs for group snapshots */
 
diff --git a/qapi-schema.json b/qapi-schema.json
index c53cb31..749a349 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1689,6 +1689,29 @@ 
             '*mode': 'NewImageMode'} }
 
 ##
+# @blockdev-snapshot-internal-sync
+#
+# Synchronously take an internal snapshot of a block device, when the format
+# of the image used supports it.
+#
+# @device: the name of the device to generate the snapshot from
+#
+# @name: #optional the new snapshot name. If not specified, a name will be
+# generated according to time by qemu
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If any snapshot matching @name exists, or the name is a numeric
+#          which may mess up with snapshot ID, generic error will be returned
+#          If the format of the image used does not support it,
+#          BlockFormatFeatureNotSupported
+#
+# Since 1.6
+##
+{ 'command': 'blockdev-snapshot-internal-sync',
+  'data': { 'device': 'str', '*name': 'str'} }
+
+##
 # @human-monitor-command:
 #
 # Execute a command on the human monitor and return the output.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index db5d4e3..3d5f996 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1044,6 +1044,37 @@  Example:
 EQMP
 
     {
+        .name       = "blockdev-snapshot-internal-sync",
+        .args_type  = "device:B,name:s?",
+        .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_internal_sync,
+    },
+
+SQMP
+blockdev-snapshot-internal-sync
+-------------------------------
+
+Synchronously take an internal snapshot of a block device when the format of
+image used supports it.  If name is not specified, it will be automatically
+generated by qemu according to host time.  If the name is a numeric string
+which may mess up with ID, such as "19", the operation will fail.  If a
+snapshot with name already exists, the operation will fail.
+
+Arguments:
+
+- "device": device name to snapshot (json-string)
+- "name": name of the new snapshot (json-string, optional)
+
+Example:
+
+-> { "execute": "blockdev-snapshot-internal-sync",
+                "arguments": { "device": "ide-hd0",
+                               "name": "snapshot0" }
+   }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "drive-mirror",
         .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
                       "on-source-error:s?,on-target-error:s?,"