[Qemu-devel] [PATCH] qapi: converted commit

[Qemu-devel] [PATCH] qapi: converted commit

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 blockdev.c       |    9 ++++-----
 blockdev.h       |    1 -
 hmp-commands.hx  |    2 +-
 hmp.c            |    9 +++++++++
 hmp.h            |    1 +
 qapi-schema.json |   15 +++++++++++++++
 qmp-commands.hx  |   24 ++++++++++++++++++++++++
 7 files changed, 54 insertions(+), 7 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 622ecba..d78af31 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -631,27 +631,26 @@ err:
     return NULL;
 }
 
-void do_commit(Monitor *mon, const QDict *qdict)
+void qmp_commit(const char *device, Error **errp)
 {
-    const char *device = qdict_get_str(qdict, "device");
     BlockDriverState *bs;
     int ret;
 
     if (!strcmp(device, "all")) {
         ret = bdrv_commit_all();
         if (ret == -EBUSY) {
-            qerror_report(QERR_DEVICE_IN_USE, device);
+            error_set(errp, QERR_DEVICE_IN_USE, device);
             return;
         }
     } else {
         bs = bdrv_find(device);
         if (!bs) {
-            qerror_report(QERR_DEVICE_NOT_FOUND, device);
+            error_set(errp, QERR_DEVICE_NOT_FOUND, device);
             return;
         }
         ret = bdrv_commit(bs);
         if (ret == -EBUSY) {
-            qerror_report(QERR_DEVICE_IN_USE, device);
+            error_set(errp, QERR_DEVICE_IN_USE, device);
             return;
         }
     }
diff --git a/blockdev.h b/blockdev.h
index 260e16b..c2e52bb 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -60,6 +60,5 @@ DriveInfo *add_init_drive(const char *opts);
 
 void qmp_change_blockdev(const char *device, const char *filename,
                          bool has_format, const char *format, Error **errp);
-void do_commit(Monitor *mon, const QDict *qdict);
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
 #endif
diff --git a/hmp-commands.hx b/hmp-commands.hx
index f5d9d91..53d2c34 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -28,7 +28,7 @@ ETEXI
         .args_type  = "device:B",
         .params     = "device|all",
         .help       = "commit changes to the disk images (if -snapshot is used) or backing files",
-        .mhandler.cmd = do_commit,
+        .mhandler.cmd = hmp_commit,
     },
 
 STEXI
diff --git a/hmp.c b/hmp.c
index 2ce8cb9..176defa 100644
--- a/hmp.c
+++ b/hmp.c
@@ -999,3 +999,12 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
     qmp_netdev_del(id, &err);
     hmp_handle_error(mon, &err);
 }
+
+void hmp_commit(Monitor *mon, const QDict *qdict)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    Error *err = NULL;
+
+    qmp_commit(device, &err);
+    hmp_handle_error(mon, &err);
+}
diff --git a/hmp.h b/hmp.h
index 79d138d..99d57c0 100644
--- a/hmp.h
+++ b/hmp.h
@@ -64,5 +64,6 @@ void hmp_device_del(Monitor *mon, const QDict *qdict);
 void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
 void hmp_netdev_add(Monitor *mon, const QDict *qdict);
 void hmp_netdev_del(Monitor *mon, const QDict *qdict);
+void hmp_commit(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 3b6e346..10cb22b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1169,6 +1169,21 @@
 { 'command': 'block_resize', 'data': { 'device': 'str', 'size': 'int' }}
 
 ##
+# @commit
+#
+# Commit changes to the disk images (if -snapshot is used) or backing files.
+#
+# @device: the name of the device or the "all" to commit all devices
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If a long-running operation is using the device, DeviceInUse
+#
+# Since: 1.1
+##
+{ 'command': 'commit', 'data': { 'device': 'str' }}
+
+##
 # @NewImageMode
 #
 # An enumeration that tells QEMU how to set the backing file path in
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 2e1a38e..8b6bfbc 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -714,6 +714,30 @@ Example:
 -> { "execute": "block_resize", "arguments": { "device": "scratch", "size": 1073741824 } }
 <- { "return": {} }
 
+
+EQMP
+
+    {
+        .name       = "commit",
+        .args_type  = "device:B",
+        .mhandler.cmd_new = qmp_marshal_input_commit,
+    },
+
+SQMP
+commit
+------
+
+Commit changes to the disk images (if -snapshot is used) or backing files.
+
+Arguments:
+
+- "device": the device's ID, or all (json-string)
+
+Example:
+
+-> { "execute": "commit", "arguments": { "device": "all" } }
+<- { "return": {} }
+
 EQMP
 
     {
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH v2] qapi: converted commit

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1672 bytes --]

On 06/14/2012 01:35 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---

> +++ b/qapi-schema.json
> @@ -1169,6 +1169,21 @@
>  { 'command': 'block_resize', 'data': { 'device': 'str', 'size': 'int' }}
>  
>  ##
> +# @commit
> +#
> +# Commit changes to the disk images (if -snapshot is used) or backing files.
> +#
> +# @device: the name of the device or the "all" to commit all devices
> +#
> +# Returns: nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#          If a long-running operation is using the device, DeviceInUse
> +#
> +# Since: 1.2
> +##
> +{ 'command': 'commit', 'data': { 'device': 'str' }}

Should we use this as an opportunity to make the command more powerful?
 For example, integrating this with the 'transaction' command or a block
job queried by 'query-block-jobs' to track its progress would be useful.
 Also, suppose I have A <- B <- C.  Does 'commit' only do one layer (C
into B), or all layers (B and C into A)?  That argues that we need an
optional parameter that says how deep to commit (committing C into B
only to repeat and commit B into A is more time-consuming than directly
committing both B and C into A to start with).  When a commit is
complete, which file is backing the device - is it still C (which
continues to diverge, but now from the point of the commit) or does qemu
pivot things to have the device now backed by B (and C can be discarded,
particularly true if changes are now going into B which invalidate C).

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH v2] qapi: converted commit

[Pavel Hrdina]

On 06/14/2012 02:18 PM, Eric Blake wrote:
> On 06/14/2012 01:35 AM, Pavel Hrdina wrote:
>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
>> ---
>> +++ b/qapi-schema.json
>> @@ -1169,6 +1169,21 @@
>>   { 'command': 'block_resize', 'data': { 'device': 'str', 'size': 'int' }}
>>
>>   ##
>> +# @commit
>> +#
>> +# Commit changes to the disk images (if -snapshot is used) or backing files.
>> +#
>> +# @device: the name of the device or the "all" to commit all devices
>> +#
>> +# Returns: nothing on success
>> +#          If @device is not a valid block device, DeviceNotFound
>> +#          If a long-running operation is using the device, DeviceInUse
>> +#
>> +# Since: 1.2
>> +##
>> +{ 'command': 'commit', 'data': { 'device': 'str' }}
> Should we use this as an opportunity to make the command more powerful?
>   For example, integrating this with the 'transaction' command or a block
> job queried by 'query-block-jobs' to track its progress would be useful.
>   Also, suppose I have A<- B<- C.  Does 'commit' only do one layer (C
> into B), or all layers (B and C into A)?  That argues that we need an
> optional parameter that says how deep to commit (committing C into B
> only to repeat and commit B into A is more time-consuming than directly
> committing both B and C into A to start with).  When a commit is
> complete, which file is backing the device - is it still C (which
> continues to diverge, but now from the point of the commit) or does qemu
> pivot things to have the device now backed by B (and C can be discarded,
> particularly true if changes are now going into B which invalidate C).

What i find out is that 'commit' will commit changes only from C to B 
and qemu continues with C from the new commit point. I couldn't find a 
way to commit changes and go back to backing file. This should be 
supported by parameter and also as you mention that commit all changes 
through all snapshots should be supported by another parameter.
The 'transaction' feature would be nice to have too.

Pavel

Re: [Qemu-devel] [PATCH v2] qapi: converted commit

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2506 bytes --]

On 06/14/2012 08:56 AM, Pavel Hrdina wrote:
> On 06/14/2012 02:18 PM, Eric Blake wrote:
>> On 06/14/2012 01:35 AM, Pavel Hrdina wrote:
>>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
>>> ---
>>> +++ b/qapi-schema.json
>>> @@ -1169,6 +1169,21 @@
>>>   { 'command': 'block_resize', 'data': { 'device': 'str', 'size':
>>> 'int' }}
>>>
>>>   ##
>>> +# @commit
>>> +#
>>> +# Commit changes to the disk images (if -snapshot is used) or
>>> backing files.
>>> +#
>>> +# @device: the name of the device or the "all" to commit all devices
>>> +#
>>> +# Returns: nothing on success
>>> +#          If @device is not a valid block device, DeviceNotFound
>>> +#          If a long-running operation is using the device, DeviceInUse
>>> +#
>>> +# Since: 1.2
>>> +##
>>> +{ 'command': 'commit', 'data': { 'device': 'str' }}
>> Should we use this as an opportunity to make the command more powerful?
>>   For example, integrating this with the 'transaction' command or a block
>> job queried by 'query-block-jobs' to track its progress would be useful.
>>   Also, suppose I have A<- B<- C.  Does 'commit' only do one layer (C
>> into B), or all layers (B and C into A)?  That argues that we need an
>> optional parameter that says how deep to commit (committing C into B
>> only to repeat and commit B into A is more time-consuming than directly
>> committing both B and C into A to start with).  When a commit is
>> complete, which file is backing the device - is it still C (which
>> continues to diverge, but now from the point of the commit) or does qemu
>> pivot things to have the device now backed by B (and C can be discarded,
>> particularly true if changes are now going into B which invalidate C).
> 
> What i find out is that 'commit' will commit changes only from C to B
> and qemu continues with C from the new commit point. I couldn't find a
> way to commit changes and go back to backing file. This should be
> supported by parameter and also as you mention that commit all changes
> through all snapshots should be supported by another parameter.
> The 'transaction' feature would be nice to have too.

Which makes it sound like we're starting to overlap with Jeff's work on
'block-commit'.

If 'block-commit' proves to be better all around at doing what we want,
do we even need to keep 'commit' in QMP, or would it be okay for HMP only?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH v2] qapi: converted commit

[Pavel Hrdina]

On 06/14/2012 05:04 PM, Eric Blake wrote:
> On 06/14/2012 08:56 AM, Pavel Hrdina wrote:
>> On 06/14/2012 02:18 PM, Eric Blake wrote:
>>> On 06/14/2012 01:35 AM, Pavel Hrdina wrote:
>>>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
>>>> ---
>>>> +++ b/qapi-schema.json
>>>> @@ -1169,6 +1169,21 @@
>>>>    { 'command': 'block_resize', 'data': { 'device': 'str', 'size':
>>>> 'int' }}
>>>>
>>>>    ##
>>>> +# @commit
>>>> +#
>>>> +# Commit changes to the disk images (if -snapshot is used) or
>>>> backing files.
>>>> +#
>>>> +# @device: the name of the device or the "all" to commit all devices
>>>> +#
>>>> +# Returns: nothing on success
>>>> +#          If @device is not a valid block device, DeviceNotFound
>>>> +#          If a long-running operation is using the device, DeviceInUse
>>>> +#
>>>> +# Since: 1.2
>>>> +##
>>>> +{ 'command': 'commit', 'data': { 'device': 'str' }}
>>> Should we use this as an opportunity to make the command more powerful?
>>>    For example, integrating this with the 'transaction' command or a block
>>> job queried by 'query-block-jobs' to track its progress would be useful.
>>>    Also, suppose I have A<- B<- C.  Does 'commit' only do one layer (C
>>> into B), or all layers (B and C into A)?  That argues that we need an
>>> optional parameter that says how deep to commit (committing C into B
>>> only to repeat and commit B into A is more time-consuming than directly
>>> committing both B and C into A to start with).  When a commit is
>>> complete, which file is backing the device - is it still C (which
>>> continues to diverge, but now from the point of the commit) or does qemu
>>> pivot things to have the device now backed by B (and C can be discarded,
>>> particularly true if changes are now going into B which invalidate C).
>> What i find out is that 'commit' will commit changes only from C to B
>> and qemu continues with C from the new commit point. I couldn't find a
>> way to commit changes and go back to backing file. This should be
>> supported by parameter and also as you mention that commit all changes
>> through all snapshots should be supported by another parameter.
>> The 'transaction' feature would be nice to have too.
> Which makes it sound like we're starting to overlap with Jeff's work on
> 'block-commit'.
>
> If 'block-commit' proves to be better all around at doing what we want,
> do we even need to keep 'commit' in QMP, or would it be okay for HMP only?
If the 'block-commit' will be better I think that we could drop the 
'commit' completely. And have only 'block-commit' for both QMP and HMP.

Pavel

Re: [Qemu-devel] [PATCH v2] qapi: converted commit

[Luiz Capitulino]

On Thu, 14 Jun 2012 09:35:21 +0200
Pavel Hrdina <phrdina@redhat.com> wrote:

> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> Please ignore previous patch, I forget to change 'Since' version.
> 
>  blockdev.c       |    9 ++++-----
>  blockdev.h       |    1 -
>  hmp-commands.hx  |    2 +-
>  hmp.c            |    9 +++++++++
>  hmp.h            |    1 +
>  qapi-schema.json |   15 +++++++++++++++
>  qmp-commands.hx  |   24 ++++++++++++++++++++++++
>  7 files changed, 54 insertions(+), 7 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 622ecba..d78af31 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -631,27 +631,26 @@ err:
>      return NULL;
>  }
>  
> -void do_commit(Monitor *mon, const QDict *qdict)
> +void qmp_commit(const char *device, Error **errp)
>  {
> -    const char *device = qdict_get_str(qdict, "device");
>      BlockDriverState *bs;
>      int ret;
>  
>      if (!strcmp(device, "all")) {

This 'all' functionality should be moved to hmp_commit(). You can call
qmp_query_block() and loop through the list of devices calling qmp_commit()
on them.

Note that we should do this even if we go for Jeff's block-commit.

>          ret = bdrv_commit_all();
>          if (ret == -EBUSY) {
> -            qerror_report(QERR_DEVICE_IN_USE, device);
> +            error_set(errp, QERR_DEVICE_IN_USE, device);
>              return;
>          }
>      } else {
>          bs = bdrv_find(device);
>          if (!bs) {
> -            qerror_report(QERR_DEVICE_NOT_FOUND, device);
> +            error_set(errp, QERR_DEVICE_NOT_FOUND, device);
>              return;
>          }
>          ret = bdrv_commit(bs);
>          if (ret == -EBUSY) {
> -            qerror_report(QERR_DEVICE_IN_USE, device);
> +            error_set(errp, QERR_DEVICE_IN_USE, device);
>              return;
>          }

bdrv_commit() can return more errors, but this will ignore them and
report success instead. We want to return qerrors for all possible
bdrv_commit() errors. But it's a good idea to wait to see if we'll use
Jeff's block-commit before making changed to this patch.

>      }
> diff --git a/blockdev.h b/blockdev.h
> index 260e16b..c2e52bb 100644
> --- a/blockdev.h
> +++ b/blockdev.h
> @@ -60,6 +60,5 @@ DriveInfo *add_init_drive(const char *opts);
>  
>  void qmp_change_blockdev(const char *device, const char *filename,
>                           bool has_format, const char *format, Error **errp);
> -void do_commit(Monitor *mon, const QDict *qdict);
>  int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  #endif
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index f5d9d91..53d2c34 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -28,7 +28,7 @@ ETEXI
>          .args_type  = "device:B",
>          .params     = "device|all",
>          .help       = "commit changes to the disk images (if -snapshot is used) or backing files",
> -        .mhandler.cmd = do_commit,
> +        .mhandler.cmd = hmp_commit,
>      },
>  
>  STEXI
> diff --git a/hmp.c b/hmp.c
> index 2ce8cb9..176defa 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -999,3 +999,12 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
>      qmp_netdev_del(id, &err);
>      hmp_handle_error(mon, &err);
>  }
> +
> +void hmp_commit(Monitor *mon, const QDict *qdict)
> +{
> +    const char *device = qdict_get_str(qdict, "device");
> +    Error *err = NULL;
> +
> +    qmp_commit(device, &err);
> +    hmp_handle_error(mon, &err);
> +}
> diff --git a/hmp.h b/hmp.h
> index 79d138d..99d57c0 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -64,5 +64,6 @@ void hmp_device_del(Monitor *mon, const QDict *qdict);
>  void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
>  void hmp_netdev_add(Monitor *mon, const QDict *qdict);
>  void hmp_netdev_del(Monitor *mon, const QDict *qdict);
> +void hmp_commit(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 3b6e346..10cb22b 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1169,6 +1169,21 @@
>  { 'command': 'block_resize', 'data': { 'device': 'str', 'size': 'int' }}
>  
>  ##
> +# @commit
> +#
> +# Commit changes to the disk images (if -snapshot is used) or backing files.
> +#
> +# @device: the name of the device or the "all" to commit all devices
> +#
> +# Returns: nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#          If a long-running operation is using the device, DeviceInUse
> +#
> +# Since: 1.2
> +##
> +{ 'command': 'commit', 'data': { 'device': 'str' }}
> +
> +##
>  # @NewImageMode
>  #
>  # An enumeration that tells QEMU how to set the backing file path in
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 2e1a38e..8b6bfbc 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -714,6 +714,30 @@ Example:
>  -> { "execute": "block_resize", "arguments": { "device": "scratch", "size": 1073741824 } }
>  <- { "return": {} }
>  
> +
> +EQMP
> +
> +    {
> +        .name       = "commit",
> +        .args_type  = "device:B",
> +        .mhandler.cmd_new = qmp_marshal_input_commit,
> +    },
> +
> +SQMP
> +commit
> +------
> +
> +Commit changes to the disk images (if -snapshot is used) or backing files.
> +
> +Arguments:
> +
> +- "device": the device's ID, or all (json-string)
> +
> +Example:
> +
> +-> { "execute": "commit", "arguments": { "device": "all" } }
> +<- { "return": {} }
> +
>  EQMP
>  
>      {

Re: [Qemu-devel] [PATCH v2] qapi: converted commit

[Luiz Capitulino]

On Thu, 14 Jun 2012 17:21:44 +0200
Pavel Hrdina <phrdina@redhat.com> wrote:

> On 06/14/2012 05:04 PM, Eric Blake wrote:
> > On 06/14/2012 08:56 AM, Pavel Hrdina wrote:
> >> On 06/14/2012 02:18 PM, Eric Blake wrote:
> >>> On 06/14/2012 01:35 AM, Pavel Hrdina wrote:
> >>>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
> >>>> ---
> >>>> +++ b/qapi-schema.json
> >>>> @@ -1169,6 +1169,21 @@
> >>>>    { 'command': 'block_resize', 'data': { 'device': 'str', 'size':
> >>>> 'int' }}
> >>>>
> >>>>    ##
> >>>> +# @commit
> >>>> +#
> >>>> +# Commit changes to the disk images (if -snapshot is used) or
> >>>> backing files.
> >>>> +#
> >>>> +# @device: the name of the device or the "all" to commit all devices
> >>>> +#
> >>>> +# Returns: nothing on success
> >>>> +#          If @device is not a valid block device, DeviceNotFound
> >>>> +#          If a long-running operation is using the device, DeviceInUse
> >>>> +#
> >>>> +# Since: 1.2
> >>>> +##
> >>>> +{ 'command': 'commit', 'data': { 'device': 'str' }}
> >>> Should we use this as an opportunity to make the command more powerful?
> >>>    For example, integrating this with the 'transaction' command or a block
> >>> job queried by 'query-block-jobs' to track its progress would be useful.
> >>>    Also, suppose I have A<- B<- C.  Does 'commit' only do one layer (C
> >>> into B), or all layers (B and C into A)?  That argues that we need an
> >>> optional parameter that says how deep to commit (committing C into B
> >>> only to repeat and commit B into A is more time-consuming than directly
> >>> committing both B and C into A to start with).  When a commit is
> >>> complete, which file is backing the device - is it still C (which
> >>> continues to diverge, but now from the point of the commit) or does qemu
> >>> pivot things to have the device now backed by B (and C can be discarded,
> >>> particularly true if changes are now going into B which invalidate C).
> >> What i find out is that 'commit' will commit changes only from C to B
> >> and qemu continues with C from the new commit point. I couldn't find a
> >> way to commit changes and go back to backing file. This should be
> >> supported by parameter and also as you mention that commit all changes
> >> through all snapshots should be supported by another parameter.
> >> The 'transaction' feature would be nice to have too.
> > Which makes it sound like we're starting to overlap with Jeff's work on
> > 'block-commit'.
> >
> > If 'block-commit' proves to be better all around at doing what we want,
> > do we even need to keep 'commit' in QMP, or would it be okay for HMP only?
> If the 'block-commit' will be better I think that we could drop the 
> 'commit' completely. And have only 'block-commit' for both QMP and HMP.

I completely agree about the QMP part, but for HMP it's a good idea to
maintain the commit command. To achieve this, we can implement hmp_commit()
in terms of block-commit.

Jeff, can you answer us here? Does block-commit supersedes the commit command
we have today?

Re: [Qemu-devel] [PATCH v2] qapi: converted commit

[Jeff Cody]

On 06/15/2012 10:02 AM, Luiz Capitulino wrote:
> On Thu, 14 Jun 2012 17:21:44 +0200
> Pavel Hrdina <phrdina@redhat.com> wrote:
> 
>> On 06/14/2012 05:04 PM, Eric Blake wrote:
>>> On 06/14/2012 08:56 AM, Pavel Hrdina wrote:
>>>> On 06/14/2012 02:18 PM, Eric Blake wrote:
>>>>> On 06/14/2012 01:35 AM, Pavel Hrdina wrote:
>>>>>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
>>>>>> ---
>>>>>> +++ b/qapi-schema.json
>>>>>> @@ -1169,6 +1169,21 @@
>>>>>>    { 'command': 'block_resize', 'data': { 'device': 'str', 'size':
>>>>>> 'int' }}
>>>>>>
>>>>>>    ##
>>>>>> +# @commit
>>>>>> +#
>>>>>> +# Commit changes to the disk images (if -snapshot is used) or
>>>>>> backing files.
>>>>>> +#
>>>>>> +# @device: the name of the device or the "all" to commit all devices
>>>>>> +#
>>>>>> +# Returns: nothing on success
>>>>>> +#          If @device is not a valid block device, DeviceNotFound
>>>>>> +#          If a long-running operation is using the device, DeviceInUse
>>>>>> +#
>>>>>> +# Since: 1.2
>>>>>> +##
>>>>>> +{ 'command': 'commit', 'data': { 'device': 'str' }}
>>>>> Should we use this as an opportunity to make the command more powerful?
>>>>>    For example, integrating this with the 'transaction' command or a block
>>>>> job queried by 'query-block-jobs' to track its progress would be useful.
>>>>>    Also, suppose I have A<- B<- C.  Does 'commit' only do one layer (C
>>>>> into B), or all layers (B and C into A)?  That argues that we need an
>>>>> optional parameter that says how deep to commit (committing C into B
>>>>> only to repeat and commit B into A is more time-consuming than directly
>>>>> committing both B and C into A to start with).  When a commit is
>>>>> complete, which file is backing the device - is it still C (which
>>>>> continues to diverge, but now from the point of the commit) or does qemu
>>>>> pivot things to have the device now backed by B (and C can be discarded,
>>>>> particularly true if changes are now going into B which invalidate C).
>>>> What i find out is that 'commit' will commit changes only from C to B
>>>> and qemu continues with C from the new commit point. I couldn't find a
>>>> way to commit changes and go back to backing file. This should be
>>>> supported by parameter and also as you mention that commit all changes
>>>> through all snapshots should be supported by another parameter.
>>>> The 'transaction' feature would be nice to have too.
>>> Which makes it sound like we're starting to overlap with Jeff's work on
>>> 'block-commit'.
>>>
>>> If 'block-commit' proves to be better all around at doing what we want,
>>> do we even need to keep 'commit' in QMP, or would it be okay for HMP only?
>> If the 'block-commit' will be better I think that we could drop the 
>> 'commit' completely. And have only 'block-commit' for both QMP and HMP.
> 
> I completely agree about the QMP part, but for HMP it's a good idea to
> maintain the commit command. To achieve this, we can implement hmp_commit()
> in terms of block-commit.
> 
> Jeff, can you answer us here? Does block-commit supersedes the commit command
> we have today?

The block-commit will supercede in functionality the commit command in
place today, but it is a live operation - as such, it will take longer
to complete, but it won't pause the guest.

In general, I think it would be safe to say that it could supersede the
current commit command, unless others see a use case for a commit
operation that completes faster yet pauses the guest.  I think being
able to rely on qemu-img to perform an offline commit would be
sufficient.

I agree on the HMP command matching the QMP command in the method used,
so that there is no confusion.

Re: [Qemu-devel] [PATCH v2] qapi: converted commit

[Luiz Capitulino]

On Fri, 15 Jun 2012 10:11:46 -0400
Jeff Cody <jcody@redhat.com> wrote:

> On 06/15/2012 10:02 AM, Luiz Capitulino wrote:
> > On Thu, 14 Jun 2012 17:21:44 +0200
> > Pavel Hrdina <phrdina@redhat.com> wrote:
> > 
> >> On 06/14/2012 05:04 PM, Eric Blake wrote:
> >>> On 06/14/2012 08:56 AM, Pavel Hrdina wrote:
> >>>> On 06/14/2012 02:18 PM, Eric Blake wrote:
> >>>>> On 06/14/2012 01:35 AM, Pavel Hrdina wrote:
> >>>>>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
> >>>>>> ---
> >>>>>> +++ b/qapi-schema.json
> >>>>>> @@ -1169,6 +1169,21 @@
> >>>>>>    { 'command': 'block_resize', 'data': { 'device': 'str', 'size':
> >>>>>> 'int' }}
> >>>>>>
> >>>>>>    ##
> >>>>>> +# @commit
> >>>>>> +#
> >>>>>> +# Commit changes to the disk images (if -snapshot is used) or
> >>>>>> backing files.
> >>>>>> +#
> >>>>>> +# @device: the name of the device or the "all" to commit all devices
> >>>>>> +#
> >>>>>> +# Returns: nothing on success
> >>>>>> +#          If @device is not a valid block device, DeviceNotFound
> >>>>>> +#          If a long-running operation is using the device, DeviceInUse
> >>>>>> +#
> >>>>>> +# Since: 1.2
> >>>>>> +##
> >>>>>> +{ 'command': 'commit', 'data': { 'device': 'str' }}
> >>>>> Should we use this as an opportunity to make the command more powerful?
> >>>>>    For example, integrating this with the 'transaction' command or a block
> >>>>> job queried by 'query-block-jobs' to track its progress would be useful.
> >>>>>    Also, suppose I have A<- B<- C.  Does 'commit' only do one layer (C
> >>>>> into B), or all layers (B and C into A)?  That argues that we need an
> >>>>> optional parameter that says how deep to commit (committing C into B
> >>>>> only to repeat and commit B into A is more time-consuming than directly
> >>>>> committing both B and C into A to start with).  When a commit is
> >>>>> complete, which file is backing the device - is it still C (which
> >>>>> continues to diverge, but now from the point of the commit) or does qemu
> >>>>> pivot things to have the device now backed by B (and C can be discarded,
> >>>>> particularly true if changes are now going into B which invalidate C).
> >>>> What i find out is that 'commit' will commit changes only from C to B
> >>>> and qemu continues with C from the new commit point. I couldn't find a
> >>>> way to commit changes and go back to backing file. This should be
> >>>> supported by parameter and also as you mention that commit all changes
> >>>> through all snapshots should be supported by another parameter.
> >>>> The 'transaction' feature would be nice to have too.
> >>> Which makes it sound like we're starting to overlap with Jeff's work on
> >>> 'block-commit'.
> >>>
> >>> If 'block-commit' proves to be better all around at doing what we want,
> >>> do we even need to keep 'commit' in QMP, or would it be okay for HMP only?
> >> If the 'block-commit' will be better I think that we could drop the 
> >> 'commit' completely. And have only 'block-commit' for both QMP and HMP.
> > 
> > I completely agree about the QMP part, but for HMP it's a good idea to
> > maintain the commit command. To achieve this, we can implement hmp_commit()
> > in terms of block-commit.
> > 
> > Jeff, can you answer us here? Does block-commit supersedes the commit command
> > we have today?
> 
> The block-commit will supercede in functionality the commit command in
> place today, but it is a live operation - as such, it will take longer
> to complete, but it won't pause the guest.

This is very nice, is this being targeted for 1.2?

Re: [Qemu-devel] [PATCH v2] qapi: converted commit

[Jeff Cody]

On 06/15/2012 10:44 AM, Luiz Capitulino wrote:
> On Fri, 15 Jun 2012 10:11:46 -0400
> Jeff Cody <jcody@redhat.com> wrote:
> 
>> On 06/15/2012 10:02 AM, Luiz Capitulino wrote:
>>> On Thu, 14 Jun 2012 17:21:44 +0200
>>> Pavel Hrdina <phrdina@redhat.com> wrote:
>>>
>>>> On 06/14/2012 05:04 PM, Eric Blake wrote:
>>>>> On 06/14/2012 08:56 AM, Pavel Hrdina wrote:
>>>>>> On 06/14/2012 02:18 PM, Eric Blake wrote:
>>>>>>> On 06/14/2012 01:35 AM, Pavel Hrdina wrote:
>>>>>>>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
>>>>>>>> ---
>>>>>>>> +++ b/qapi-schema.json
>>>>>>>> @@ -1169,6 +1169,21 @@
>>>>>>>>    { 'command': 'block_resize', 'data': { 'device': 'str', 'size':
>>>>>>>> 'int' }}
>>>>>>>>
>>>>>>>>    ##
>>>>>>>> +# @commit
>>>>>>>> +#
>>>>>>>> +# Commit changes to the disk images (if -snapshot is used) or
>>>>>>>> backing files.
>>>>>>>> +#
>>>>>>>> +# @device: the name of the device or the "all" to commit all devices
>>>>>>>> +#
>>>>>>>> +# Returns: nothing on success
>>>>>>>> +#          If @device is not a valid block device, DeviceNotFound
>>>>>>>> +#          If a long-running operation is using the device, DeviceInUse
>>>>>>>> +#
>>>>>>>> +# Since: 1.2
>>>>>>>> +##
>>>>>>>> +{ 'command': 'commit', 'data': { 'device': 'str' }}
>>>>>>> Should we use this as an opportunity to make the command more powerful?
>>>>>>>    For example, integrating this with the 'transaction' command or a block
>>>>>>> job queried by 'query-block-jobs' to track its progress would be useful.
>>>>>>>    Also, suppose I have A<- B<- C.  Does 'commit' only do one layer (C
>>>>>>> into B), or all layers (B and C into A)?  That argues that we need an
>>>>>>> optional parameter that says how deep to commit (committing C into B
>>>>>>> only to repeat and commit B into A is more time-consuming than directly
>>>>>>> committing both B and C into A to start with).  When a commit is
>>>>>>> complete, which file is backing the device - is it still C (which
>>>>>>> continues to diverge, but now from the point of the commit) or does qemu
>>>>>>> pivot things to have the device now backed by B (and C can be discarded,
>>>>>>> particularly true if changes are now going into B which invalidate C).
>>>>>> What i find out is that 'commit' will commit changes only from C to B
>>>>>> and qemu continues with C from the new commit point. I couldn't find a
>>>>>> way to commit changes and go back to backing file. This should be
>>>>>> supported by parameter and also as you mention that commit all changes
>>>>>> through all snapshots should be supported by another parameter.
>>>>>> The 'transaction' feature would be nice to have too.
>>>>> Which makes it sound like we're starting to overlap with Jeff's work on
>>>>> 'block-commit'.
>>>>>
>>>>> If 'block-commit' proves to be better all around at doing what we want,
>>>>> do we even need to keep 'commit' in QMP, or would it be okay for HMP only?
>>>> If the 'block-commit' will be better I think that we could drop the 
>>>> 'commit' completely. And have only 'block-commit' for both QMP and HMP.
>>>
>>> I completely agree about the QMP part, but for HMP it's a good idea to
>>> maintain the commit command. To achieve this, we can implement hmp_commit()
>>> in terms of block-commit.
>>>
>>> Jeff, can you answer us here? Does block-commit supersedes the commit command
>>> we have today?
>>
>> The block-commit will supercede in functionality the commit command in
>> place today, but it is a live operation - as such, it will take longer
>> to complete, but it won't pause the guest.
> 
> This is very nice, is this being targeted for 1.2?

Yes, I'd like to see this in 1.2, so that is my goal.