[Qemu-devel] [PATCH 0/3] Group Live Snapshots

[Qemu-devel] [PATCH 0/3] Group Live Snapshots

[Jeff Cody]

This series of patches adds the ability to take group live snapshots, as 
opposed to indvidual device snapshots.  In addition, it adds the ability to
pass an array input parameter to a QMP command, although without argument 
validation.

Currently, when taking live snapshots of devices, if a device fails then the
snapshot command re-opens the original image. The problem with this approach
is that if multiple devices need to have snapshots taken together, if any
of the snapshots after the first device fails, then the devices are left in
and inconsistent state with respect to each other (since some of them have
been reverted to the original image, while others have not).

This series introduces a new command to address this issue, which takes
snapshots of a group of disks.  An arbitrary number of disks may be passed in
as a list / JSON array, and blockdev-group-snapshot-sync will take a snapshot
of that group atomically (with respect to the command).  If any disk fails the
snapshot, then all disks in the group are reverted back to the original images.
This allows the drives to remain consistent.

Another command is also introduced, that will return a list of strings, each
string corresponding to a filename of any failed image opens, including failure
to reopen the original image.  This list corresponds to the last invocation
of the command blockdev-query-group-snapshot-failure; that is, it can be called
any time after a group live snapshot, and see a list of all failed opens.  If
there were no failures, it returns an empty list.


Jeff Cody (3):
  qapi: Allow QMP/QAPI commands to have array inputs
  qapi: Introduce blockdev-group-snapshot-sync command
  qapi: Introduce blockdev-query-group-snapshot-failure

 blockdev.c       |  170 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 monitor.c        |   72 +++++++++++++++++++----
 monitor.h        |    1 +
 qapi-schema.json |   70 ++++++++++++++++++++++
 qmp-commands.hx  |   44 ++++++++++++++
 5 files changed, 345 insertions(+), 12 deletions(-)

-- 
1.7.9.rc2.1.g69204

[Qemu-devel] [PATCH 1/3] qapi: Allow QMP/QAPI commands to have array inputs

[Jeff Cody]

The QAPI scripts allow for generating commands that receive parameter
input consisting of a list of custom structs, but the QMP input paramter
checking did not support receiving a qlist as opposed to a qdict for input.

This patch allows for array input parameters, although no argument validation
is performed for commands that accept a list input.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 monitor.c |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++----------
 monitor.h |    1 +
 2 files changed, 61 insertions(+), 12 deletions(-)

diff --git a/monitor.c b/monitor.c
index a7df995..98e6017 100644
--- a/monitor.c
+++ b/monitor.c
@@ -125,8 +125,12 @@ typedef struct mon_cmd_t {
         void (*info)(Monitor *mon);
         void (*cmd)(Monitor *mon, const QDict *qdict);
         int  (*cmd_new)(Monitor *mon, const QDict *params, QObject **ret_data);
+        int  (*cmd_new_list)(Monitor *mon, const QList *params,
+                             QObject **ret_data);
         int  (*cmd_async)(Monitor *mon, const QDict *params,
                           MonitorCompletion *cb, void *opaque);
+        int  (*cmd_async_list)(Monitor *mon, const QList *params,
+                          MonitorCompletion *cb, void *opaque);
     } mhandler;
     bool qapi;
     int flags;
@@ -353,6 +357,11 @@ static inline bool handler_is_async(const mon_cmd_t *cmd)
     return cmd->flags & MONITOR_CMD_ASYNC;
 }
 
+static inline bool handler_accepts_array(const mon_cmd_t *cmd)
+{
+    return cmd->flags & MONITOR_CMD_ARRAY_INPUT;
+}
+
 static inline int monitor_has_error(const Monitor *mon)
 {
     return mon->error != NULL;
@@ -671,6 +680,12 @@ static int qmp_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd,
     return cmd->mhandler.cmd_async(mon, params, qmp_monitor_complete, mon);
 }
 
+static int qmp_async_cmd_handler_array(Monitor *mon, const mon_cmd_t *cmd,
+                                 const QList *params)
+{
+    return cmd->mhandler.cmd_async_list(mon, params, qmp_monitor_complete, mon);
+}
+
 static void user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd,
                                    const QDict *params)
 {
@@ -4310,7 +4325,8 @@ static QDict *qmp_check_input_obj(QObject *input_obj)
             }
             has_exec_key = 1;
         } else if (!strcmp(arg_name, "arguments")) {
-            if (qobject_type(arg_obj) != QTYPE_QDICT) {
+            if ((qobject_type(arg_obj) != QTYPE_QDICT) &&
+                (qobject_type(arg_obj) != QTYPE_QLIST)) {
                 qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "arguments",
                               "object");
                 return NULL;
@@ -4345,11 +4361,26 @@ static void qmp_call_cmd(Monitor *mon, const mon_cmd_t *cmd,
     qobject_decref(data);
 }
 
+static void qmp_call_cmd_array(Monitor *mon, const mon_cmd_t *cmd,
+                              const QList *params)
+{
+    int ret;
+    QObject *data = NULL;
+
+    mon_print_count_init(mon);
+
+    ret = cmd->mhandler.cmd_new_list(mon, params, &data);
+    handler_audit(mon, cmd, ret);
+    monitor_protocol_emitter(mon, data);
+    qobject_decref(data);
+}
+
 static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
 {
     int err;
     QObject *obj;
     QDict *input, *args;
+    QList *args_list = NULL;
     const mon_cmd_t *cmd;
     const char *cmd_name;
     Monitor *mon = cur_mon;
@@ -4386,26 +4417,42 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
     }
 
     obj = qdict_get(input, "arguments");
-    if (!obj) {
-        args = qdict_new();
+    if (handler_accepts_array(cmd)) {
+        if (!obj || (qobject_type(obj) != QTYPE_QLIST)) {
+            args_list = qlist_new();
+        } else {
+            args_list = qobject_to_qlist(obj);
+            QINCREF(args_list);
+        }
     } else {
-        args = qobject_to_qdict(obj);
-        QINCREF(args);
-    }
-
-    err = qmp_check_client_args(cmd, args);
-    if (err < 0) {
-        goto err_out;
+        if (!obj || (qobject_type(obj) != QTYPE_QDICT)) {
+            args = qdict_new();
+        } else {
+            args = qobject_to_qdict(obj);
+            QINCREF(args);
+        }
+        err = qmp_check_client_args(cmd, args);
+        if (err < 0) {
+            goto err_out;
+        }
     }
 
     if (handler_is_async(cmd)) {
-        err = qmp_async_cmd_handler(mon, cmd, args);
+        if (handler_accepts_array(cmd)) {
+            err = qmp_async_cmd_handler_array(mon, cmd, args_list);
+        } else {
+            err = qmp_async_cmd_handler(mon, cmd, args);
+        }
         if (err) {
             /* emit the error response */
             goto err_out;
         }
     } else {
-        qmp_call_cmd(mon, cmd, args);
+        if (handler_accepts_array(cmd)) {
+            qmp_call_cmd_array(mon, cmd, args_list);
+        } else {
+            qmp_call_cmd(mon, cmd, args);
+        }
     }
 
     goto out;
@@ -4415,6 +4462,7 @@ err_out:
 out:
     QDECREF(input);
     QDECREF(args);
+    QDECREF(args_list);
 }
 
 /**
diff --git a/monitor.h b/monitor.h
index b72ea07..499fc1f 100644
--- a/monitor.h
+++ b/monitor.h
@@ -19,6 +19,7 @@ extern Monitor *default_mon;
 
 /* flags for monitor commands */
 #define MONITOR_CMD_ASYNC       0x0001
+#define MONITOR_CMD_ARRAY_INPUT 0x0002
 
 /* QMP events */
 typedef enum MonitorEvent {
-- 
1.7.9.rc2.1.g69204

Re: [Qemu-devel] [PATCH 1/3] qapi: Allow QMP/QAPI commands to have array inputs

[Anthony Liguori]

On 02/20/2012 11:31 AM, Jeff Cody wrote:
> The QAPI scripts allow for generating commands that receive parameter
> input consisting of a list of custom structs, but the QMP input paramter
> checking did not support receiving a qlist as opposed to a qdict for input.

What are you trying to send on the wire?  Something like

{"execute": "foo", "arguments": [ "a", "b", "c" ]}

?

That's not valid per the QMP protocol.  "arguments" must always be a QMP dictionary.

Regards,

Anthony Liguori

>
> This patch allows for array input parameters, although no argument validation
> is performed for commands that accept a list input.
>
> Signed-off-by: Jeff Cody<jcody@redhat.com>
> ---
>   monitor.c |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++----------
>   monitor.h |    1 +
>   2 files changed, 61 insertions(+), 12 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index a7df995..98e6017 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -125,8 +125,12 @@ typedef struct mon_cmd_t {
>           void (*info)(Monitor *mon);
>           void (*cmd)(Monitor *mon, const QDict *qdict);
>           int  (*cmd_new)(Monitor *mon, const QDict *params, QObject **ret_data);
> +        int  (*cmd_new_list)(Monitor *mon, const QList *params,
> +                             QObject **ret_data);
>           int  (*cmd_async)(Monitor *mon, const QDict *params,
>                             MonitorCompletion *cb, void *opaque);
> +        int  (*cmd_async_list)(Monitor *mon, const QList *params,
> +                          MonitorCompletion *cb, void *opaque);
>       } mhandler;
>       bool qapi;
>       int flags;
> @@ -353,6 +357,11 @@ static inline bool handler_is_async(const mon_cmd_t *cmd)
>       return cmd->flags&  MONITOR_CMD_ASYNC;
>   }
>
> +static inline bool handler_accepts_array(const mon_cmd_t *cmd)
> +{
> +    return cmd->flags&  MONITOR_CMD_ARRAY_INPUT;
> +}
> +
>   static inline int monitor_has_error(const Monitor *mon)
>   {
>       return mon->error != NULL;
> @@ -671,6 +680,12 @@ static int qmp_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd,
>       return cmd->mhandler.cmd_async(mon, params, qmp_monitor_complete, mon);
>   }
>
> +static int qmp_async_cmd_handler_array(Monitor *mon, const mon_cmd_t *cmd,
> +                                 const QList *params)
> +{
> +    return cmd->mhandler.cmd_async_list(mon, params, qmp_monitor_complete, mon);
> +}
> +
>   static void user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd,
>                                      const QDict *params)
>   {
> @@ -4310,7 +4325,8 @@ static QDict *qmp_check_input_obj(QObject *input_obj)
>               }
>               has_exec_key = 1;
>           } else if (!strcmp(arg_name, "arguments")) {
> -            if (qobject_type(arg_obj) != QTYPE_QDICT) {
> +            if ((qobject_type(arg_obj) != QTYPE_QDICT)&&
> +                (qobject_type(arg_obj) != QTYPE_QLIST)) {
>                   qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "arguments",
>                                 "object");
>                   return NULL;
> @@ -4345,11 +4361,26 @@ static void qmp_call_cmd(Monitor *mon, const mon_cmd_t *cmd,
>       qobject_decref(data);
>   }
>
> +static void qmp_call_cmd_array(Monitor *mon, const mon_cmd_t *cmd,
> +                              const QList *params)
> +{
> +    int ret;
> +    QObject *data = NULL;
> +
> +    mon_print_count_init(mon);
> +
> +    ret = cmd->mhandler.cmd_new_list(mon, params,&data);
> +    handler_audit(mon, cmd, ret);
> +    monitor_protocol_emitter(mon, data);
> +    qobject_decref(data);
> +}
> +
>   static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>   {
>       int err;
>       QObject *obj;
>       QDict *input, *args;
> +    QList *args_list = NULL;
>       const mon_cmd_t *cmd;
>       const char *cmd_name;
>       Monitor *mon = cur_mon;
> @@ -4386,26 +4417,42 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>       }
>
>       obj = qdict_get(input, "arguments");
> -    if (!obj) {
> -        args = qdict_new();
> +    if (handler_accepts_array(cmd)) {
> +        if (!obj || (qobject_type(obj) != QTYPE_QLIST)) {
> +            args_list = qlist_new();
> +        } else {
> +            args_list = qobject_to_qlist(obj);
> +            QINCREF(args_list);
> +        }
>       } else {
> -        args = qobject_to_qdict(obj);
> -        QINCREF(args);
> -    }
> -
> -    err = qmp_check_client_args(cmd, args);
> -    if (err<  0) {
> -        goto err_out;
> +        if (!obj || (qobject_type(obj) != QTYPE_QDICT)) {
> +            args = qdict_new();
> +        } else {
> +            args = qobject_to_qdict(obj);
> +            QINCREF(args);
> +        }
> +        err = qmp_check_client_args(cmd, args);
> +        if (err<  0) {
> +            goto err_out;
> +        }
>       }
>
>       if (handler_is_async(cmd)) {
> -        err = qmp_async_cmd_handler(mon, cmd, args);
> +        if (handler_accepts_array(cmd)) {
> +            err = qmp_async_cmd_handler_array(mon, cmd, args_list);
> +        } else {
> +            err = qmp_async_cmd_handler(mon, cmd, args);
> +        }
>           if (err) {
>               /* emit the error response */
>               goto err_out;
>           }
>       } else {
> -        qmp_call_cmd(mon, cmd, args);
> +        if (handler_accepts_array(cmd)) {
> +            qmp_call_cmd_array(mon, cmd, args_list);
> +        } else {
> +            qmp_call_cmd(mon, cmd, args);
> +        }
>       }
>
>       goto out;
> @@ -4415,6 +4462,7 @@ err_out:
>   out:
>       QDECREF(input);
>       QDECREF(args);
> +    QDECREF(args_list);
>   }
>
>   /**
> diff --git a/monitor.h b/monitor.h
> index b72ea07..499fc1f 100644
> --- a/monitor.h
> +++ b/monitor.h
> @@ -19,6 +19,7 @@ extern Monitor *default_mon;
>
>   /* flags for monitor commands */
>   #define MONITOR_CMD_ASYNC       0x0001
> +#define MONITOR_CMD_ARRAY_INPUT 0x0002
>
>   /* QMP events */
>   typedef enum MonitorEvent {

Re: [Qemu-devel] [PATCH 1/3] qapi: Allow QMP/QAPI commands to have array inputs

[Jeff Cody]

On 02/22/2012 09:53 AM, Anthony Liguori wrote:
> On 02/20/2012 11:31 AM, Jeff Cody wrote:
>> The QAPI scripts allow for generating commands that receive parameter
>> input consisting of a list of custom structs, but the QMP input paramter
>> checking did not support receiving a qlist as opposed to a qdict for
>> input.
>
> What are you trying to send on the wire? Something like
>
> {"execute": "foo", "arguments": [ "a", "b", "c" ]}
>
> ?
>
> That's not valid per the QMP protocol. "arguments" must always be a QMP
> dictionary.
>

Not a bare list like that (perhaps my wording was poor), but rather an 
array of custom structs, that contain QMP dicts.  In this particular case:

{"execute": "blockdev-group-snapshot-sync", "arguments": [{ "device": 
"ide-hd0","snapshot-file":"/some/place/my-image","format": "qcow2" }]}

When specifying this in the schema JSON file as shown below[1], the QAPI 
scripts generate the marshaller & visitor functions that handle 
everything correctly.  The monitor code, however, could not deal with 
the QList container for the input parameters, prior to passing the data 
to the generated functions.


[1] JSON schema definition:

{ 'type': 'SnapshotDev',
   'data': {'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }

{ 'command': 'blockdev-group-snapshot-sync',
   'data': { 'dev': [ 'SnapshotDev' ] } }



>>
>> This patch allows for array input parameters, although no argument
>> validation
>> is performed for commands that accept a list input.
>>
>> Signed-off-by: Jeff Cody<jcody@redhat.com>
>> ---
>> monitor.c | 72
>> ++++++++++++++++++++++++++++++++++++++++++++++++++----------
>> monitor.h | 1 +
>> 2 files changed, 61 insertions(+), 12 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index a7df995..98e6017 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -125,8 +125,12 @@ typedef struct mon_cmd_t {
>> void (*info)(Monitor *mon);
>> void (*cmd)(Monitor *mon, const QDict *qdict);
>> int (*cmd_new)(Monitor *mon, const QDict *params, QObject **ret_data);
>> + int (*cmd_new_list)(Monitor *mon, const QList *params,
>> + QObject **ret_data);
>> int (*cmd_async)(Monitor *mon, const QDict *params,
>> MonitorCompletion *cb, void *opaque);
>> + int (*cmd_async_list)(Monitor *mon, const QList *params,
>> + MonitorCompletion *cb, void *opaque);
>> } mhandler;
>> bool qapi;
>> int flags;
>> @@ -353,6 +357,11 @@ static inline bool handler_is_async(const
>> mon_cmd_t *cmd)
>> return cmd->flags& MONITOR_CMD_ASYNC;
>> }
>>
>> +static inline bool handler_accepts_array(const mon_cmd_t *cmd)
>> +{
>> + return cmd->flags& MONITOR_CMD_ARRAY_INPUT;
>> +}
>> +
>> static inline int monitor_has_error(const Monitor *mon)
>> {
>> return mon->error != NULL;
>> @@ -671,6 +680,12 @@ static int qmp_async_cmd_handler(Monitor *mon,
>> const mon_cmd_t *cmd,
>> return cmd->mhandler.cmd_async(mon, params, qmp_monitor_complete, mon);
>> }
>>
>> +static int qmp_async_cmd_handler_array(Monitor *mon, const mon_cmd_t
>> *cmd,
>> + const QList *params)
>> +{
>> + return cmd->mhandler.cmd_async_list(mon, params,
>> qmp_monitor_complete, mon);
>> +}
>> +
>> static void user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd,
>> const QDict *params)
>> {
>> @@ -4310,7 +4325,8 @@ static QDict *qmp_check_input_obj(QObject
>> *input_obj)
>> }
>> has_exec_key = 1;
>> } else if (!strcmp(arg_name, "arguments")) {
>> - if (qobject_type(arg_obj) != QTYPE_QDICT) {
>> + if ((qobject_type(arg_obj) != QTYPE_QDICT)&&
>> + (qobject_type(arg_obj) != QTYPE_QLIST)) {
>> qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "arguments",
>> "object");
>> return NULL;
>> @@ -4345,11 +4361,26 @@ static void qmp_call_cmd(Monitor *mon, const
>> mon_cmd_t *cmd,
>> qobject_decref(data);
>> }
>>
>> +static void qmp_call_cmd_array(Monitor *mon, const mon_cmd_t *cmd,
>> + const QList *params)
>> +{
>> + int ret;
>> + QObject *data = NULL;
>> +
>> + mon_print_count_init(mon);
>> +
>> + ret = cmd->mhandler.cmd_new_list(mon, params,&data);
>> + handler_audit(mon, cmd, ret);
>> + monitor_protocol_emitter(mon, data);
>> + qobject_decref(data);
>> +}
>> +
>> static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>> {
>> int err;
>> QObject *obj;
>> QDict *input, *args;
>> + QList *args_list = NULL;
>> const mon_cmd_t *cmd;
>> const char *cmd_name;
>> Monitor *mon = cur_mon;
>> @@ -4386,26 +4417,42 @@ static void
>> handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>> }
>>
>> obj = qdict_get(input, "arguments");
>> - if (!obj) {
>> - args = qdict_new();
>> + if (handler_accepts_array(cmd)) {
>> + if (!obj || (qobject_type(obj) != QTYPE_QLIST)) {
>> + args_list = qlist_new();
>> + } else {
>> + args_list = qobject_to_qlist(obj);
>> + QINCREF(args_list);
>> + }
>> } else {
>> - args = qobject_to_qdict(obj);
>> - QINCREF(args);
>> - }
>> -
>> - err = qmp_check_client_args(cmd, args);
>> - if (err< 0) {
>> - goto err_out;
>> + if (!obj || (qobject_type(obj) != QTYPE_QDICT)) {
>> + args = qdict_new();
>> + } else {
>> + args = qobject_to_qdict(obj);
>> + QINCREF(args);
>> + }
>> + err = qmp_check_client_args(cmd, args);
>> + if (err< 0) {
>> + goto err_out;
>> + }
>> }
>>
>> if (handler_is_async(cmd)) {
>> - err = qmp_async_cmd_handler(mon, cmd, args);
>> + if (handler_accepts_array(cmd)) {
>> + err = qmp_async_cmd_handler_array(mon, cmd, args_list);
>> + } else {
>> + err = qmp_async_cmd_handler(mon, cmd, args);
>> + }
>> if (err) {
>> /* emit the error response */
>> goto err_out;
>> }
>> } else {
>> - qmp_call_cmd(mon, cmd, args);
>> + if (handler_accepts_array(cmd)) {
>> + qmp_call_cmd_array(mon, cmd, args_list);
>> + } else {
>> + qmp_call_cmd(mon, cmd, args);
>> + }
>> }
>>
>> goto out;
>> @@ -4415,6 +4462,7 @@ err_out:
>> out:
>> QDECREF(input);
>> QDECREF(args);
>> + QDECREF(args_list);
>> }
>>
>> /**
>> diff --git a/monitor.h b/monitor.h
>> index b72ea07..499fc1f 100644
>> --- a/monitor.h
>> +++ b/monitor.h
>> @@ -19,6 +19,7 @@ extern Monitor *default_mon;
>>
>> /* flags for monitor commands */
>> #define MONITOR_CMD_ASYNC 0x0001
>> +#define MONITOR_CMD_ARRAY_INPUT 0x0002
>>
>> /* QMP events */
>> typedef enum MonitorEvent {
>

Re: [Qemu-devel] [PATCH 1/3] qapi: Allow QMP/QAPI commands to have array inputs

[Anthony Liguori]

On 02/22/2012 10:12 AM, Jeff Cody wrote:
> On 02/22/2012 09:53 AM, Anthony Liguori wrote:
>> On 02/20/2012 11:31 AM, Jeff Cody wrote:
>>> The QAPI scripts allow for generating commands that receive parameter
>>> input consisting of a list of custom structs, but the QMP input paramter
>>> checking did not support receiving a qlist as opposed to a qdict for
>>> input.
>>
>> What are you trying to send on the wire? Something like
>>
>> {"execute": "foo", "arguments": [ "a", "b", "c" ]}
>>
>> ?
>>
>> That's not valid per the QMP protocol. "arguments" must always be a QMP
>> dictionary.
>>
>
> Not a bare list like that (perhaps my wording was poor), but rather an array of
> custom structs, that contain QMP dicts. In this particular case:
>
> {"execute": "blockdev-group-snapshot-sync", "arguments": [{ "device":
> "ide-hd0","snapshot-file":"/some/place/my-image","format": "qcow2" }]}
>
> When specifying this in the schema JSON file as shown below[1], the QAPI scripts
> generate the marshaller & visitor functions that handle everything correctly.
> The monitor code, however, could not deal with the QList container for the input
> parameters, prior to passing the data to the generated functions.
>
>
> [1] JSON schema definition:
>
> { 'type': 'SnapshotDev',
> 'data': {'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
>
> { 'command': 'blockdev-group-snapshot-sync',
> 'data': { 'dev': [ 'SnapshotDev' ] } }


This will end up looking like:

{ "execute": "blockdev-group-snapshot-sync",
   "arguments": { "dev": [{"device": "ide-hd0"....}] } }

Which should work as expected in QMP.  HMP argument validation doesn't handle 
arguments of lists but IMHO, it's time to kill that code.

Luiz, what do you think?

Regards,

Anthony Liguori

>
>
>
>>>
>>> This patch allows for array input parameters, although no argument
>>> validation
>>> is performed for commands that accept a list input.
>>>
>>> Signed-off-by: Jeff Cody<jcody@redhat.com>
>>> ---
>>> monitor.c | 72
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++----------
>>> monitor.h | 1 +
>>> 2 files changed, 61 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/monitor.c b/monitor.c
>>> index a7df995..98e6017 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -125,8 +125,12 @@ typedef struct mon_cmd_t {
>>> void (*info)(Monitor *mon);
>>> void (*cmd)(Monitor *mon, const QDict *qdict);
>>> int (*cmd_new)(Monitor *mon, const QDict *params, QObject **ret_data);
>>> + int (*cmd_new_list)(Monitor *mon, const QList *params,
>>> + QObject **ret_data);
>>> int (*cmd_async)(Monitor *mon, const QDict *params,
>>> MonitorCompletion *cb, void *opaque);
>>> + int (*cmd_async_list)(Monitor *mon, const QList *params,
>>> + MonitorCompletion *cb, void *opaque);
>>> } mhandler;
>>> bool qapi;
>>> int flags;
>>> @@ -353,6 +357,11 @@ static inline bool handler_is_async(const
>>> mon_cmd_t *cmd)
>>> return cmd->flags& MONITOR_CMD_ASYNC;
>>> }
>>>
>>> +static inline bool handler_accepts_array(const mon_cmd_t *cmd)
>>> +{
>>> + return cmd->flags& MONITOR_CMD_ARRAY_INPUT;
>>> +}
>>> +
>>> static inline int monitor_has_error(const Monitor *mon)
>>> {
>>> return mon->error != NULL;
>>> @@ -671,6 +680,12 @@ static int qmp_async_cmd_handler(Monitor *mon,
>>> const mon_cmd_t *cmd,
>>> return cmd->mhandler.cmd_async(mon, params, qmp_monitor_complete, mon);
>>> }
>>>
>>> +static int qmp_async_cmd_handler_array(Monitor *mon, const mon_cmd_t
>>> *cmd,
>>> + const QList *params)
>>> +{
>>> + return cmd->mhandler.cmd_async_list(mon, params,
>>> qmp_monitor_complete, mon);
>>> +}
>>> +
>>> static void user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd,
>>> const QDict *params)
>>> {
>>> @@ -4310,7 +4325,8 @@ static QDict *qmp_check_input_obj(QObject
>>> *input_obj)
>>> }
>>> has_exec_key = 1;
>>> } else if (!strcmp(arg_name, "arguments")) {
>>> - if (qobject_type(arg_obj) != QTYPE_QDICT) {
>>> + if ((qobject_type(arg_obj) != QTYPE_QDICT)&&
>>> + (qobject_type(arg_obj) != QTYPE_QLIST)) {
>>> qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "arguments",
>>> "object");
>>> return NULL;
>>> @@ -4345,11 +4361,26 @@ static void qmp_call_cmd(Monitor *mon, const
>>> mon_cmd_t *cmd,
>>> qobject_decref(data);
>>> }
>>>
>>> +static void qmp_call_cmd_array(Monitor *mon, const mon_cmd_t *cmd,
>>> + const QList *params)
>>> +{
>>> + int ret;
>>> + QObject *data = NULL;
>>> +
>>> + mon_print_count_init(mon);
>>> +
>>> + ret = cmd->mhandler.cmd_new_list(mon, params,&data);
>>> + handler_audit(mon, cmd, ret);
>>> + monitor_protocol_emitter(mon, data);
>>> + qobject_decref(data);
>>> +}
>>> +
>>> static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>>> {
>>> int err;
>>> QObject *obj;
>>> QDict *input, *args;
>>> + QList *args_list = NULL;
>>> const mon_cmd_t *cmd;
>>> const char *cmd_name;
>>> Monitor *mon = cur_mon;
>>> @@ -4386,26 +4417,42 @@ static void
>>> handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>>> }
>>>
>>> obj = qdict_get(input, "arguments");
>>> - if (!obj) {
>>> - args = qdict_new();
>>> + if (handler_accepts_array(cmd)) {
>>> + if (!obj || (qobject_type(obj) != QTYPE_QLIST)) {
>>> + args_list = qlist_new();
>>> + } else {
>>> + args_list = qobject_to_qlist(obj);
>>> + QINCREF(args_list);
>>> + }
>>> } else {
>>> - args = qobject_to_qdict(obj);
>>> - QINCREF(args);
>>> - }
>>> -
>>> - err = qmp_check_client_args(cmd, args);
>>> - if (err< 0) {
>>> - goto err_out;
>>> + if (!obj || (qobject_type(obj) != QTYPE_QDICT)) {
>>> + args = qdict_new();
>>> + } else {
>>> + args = qobject_to_qdict(obj);
>>> + QINCREF(args);
>>> + }
>>> + err = qmp_check_client_args(cmd, args);
>>> + if (err< 0) {
>>> + goto err_out;
>>> + }
>>> }
>>>
>>> if (handler_is_async(cmd)) {
>>> - err = qmp_async_cmd_handler(mon, cmd, args);
>>> + if (handler_accepts_array(cmd)) {
>>> + err = qmp_async_cmd_handler_array(mon, cmd, args_list);
>>> + } else {
>>> + err = qmp_async_cmd_handler(mon, cmd, args);
>>> + }
>>> if (err) {
>>> /* emit the error response */
>>> goto err_out;
>>> }
>>> } else {
>>> - qmp_call_cmd(mon, cmd, args);
>>> + if (handler_accepts_array(cmd)) {
>>> + qmp_call_cmd_array(mon, cmd, args_list);
>>> + } else {
>>> + qmp_call_cmd(mon, cmd, args);
>>> + }
>>> }
>>>
>>> goto out;
>>> @@ -4415,6 +4462,7 @@ err_out:
>>> out:
>>> QDECREF(input);
>>> QDECREF(args);
>>> + QDECREF(args_list);
>>> }
>>>
>>> /**
>>> diff --git a/monitor.h b/monitor.h
>>> index b72ea07..499fc1f 100644
>>> --- a/monitor.h
>>> +++ b/monitor.h
>>> @@ -19,6 +19,7 @@ extern Monitor *default_mon;
>>>
>>> /* flags for monitor commands */
>>> #define MONITOR_CMD_ASYNC 0x0001
>>> +#define MONITOR_CMD_ARRAY_INPUT 0x0002
>>>
>>> /* QMP events */
>>> typedef enum MonitorEvent {
>>
>

Re: [Qemu-devel] [PATCH 1/3] qapi: Allow QMP/QAPI commands to have array inputs

[Eric Blake]

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

On 02/22/2012 10:35 AM, Anthony Liguori wrote:

>> [1] JSON schema definition:
>>
>> { 'type': 'SnapshotDev',
>> 'data': {'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
>>
>> { 'command': 'blockdev-group-snapshot-sync',
>> 'data': { 'dev': [ 'SnapshotDev' ] } }
> 
> 
> This will end up looking like:
> 
> { "execute": "blockdev-group-snapshot-sync",
>   "arguments": { "dev": [{"device": "ide-hd0"....}] } }

Does that mean we want it to look more like:

{ 'command': 'blockdev-group-snapshot-sync',
  'data': { 'devlist': [ 'SnapshotDev' ] } }

and used like:

{ "execute": "blockdev-group-snapshot-sync",
  "arguments": { "devlist": [{ "device": "ide-hd0" ...},
                             { "device": "ide-hd1" ...}] } }

I don't quite care what syntax we use, as long as we settle on something
soon, because I'm trying to code up libvirt to generate this new monitor
command, and need to know what syntax it will use.

-- 
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 1/3] qapi: Allow QMP/QAPI commands to have array inputs

[Anthony Liguori]

On 02/22/2012 11:47 AM, Eric Blake wrote:
> On 02/22/2012 10:35 AM, Anthony Liguori wrote:
>
>>> [1] JSON schema definition:
>>>
>>> { 'type': 'SnapshotDev',
>>> 'data': {'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
>>>
>>> { 'command': 'blockdev-group-snapshot-sync',
>>> 'data': { 'dev': [ 'SnapshotDev' ] } }
>>
>>
>> This will end up looking like:
>>
>> { "execute": "blockdev-group-snapshot-sync",
>>    "arguments": { "dev": [{"device": "ide-hd0"....}] } }
>
> Does that mean we want it to look more like:
>
> { 'command': 'blockdev-group-snapshot-sync',
>    'data': { 'devlist': [ 'SnapshotDev' ] } }
>
> and used like:
>
> { "execute": "blockdev-group-snapshot-sync",
>    "arguments": { "devlist": [{ "device": "ide-hd0" ...},
>                               { "device": "ide-hd1" ...}] } }


Yes, devlist is a better variable name.

>
> I don't quite care what syntax we use, as long as we settle on something
> soon, because I'm trying to code up libvirt to generate this new monitor
> command, and need to know what syntax it will use.


Just FYI, I've looked at the QAPI bits so far.  I would suggest getting an Ack 
from Kevin at least that this API is reasonable inline with what we're looking 
for here before you start doing libvirt support.

Personally, I loathe the idea of extending an already broken interface...  I 
would rather we fix the original interface first before tacking on additional 
features.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/3] qapi: Allow QMP/QAPI commands to have array inputs

[Jeff Cody]

On 02/22/2012 12:56 PM, Anthony Liguori wrote:
> On 02/22/2012 11:47 AM, Eric Blake wrote:
>> On 02/22/2012 10:35 AM, Anthony Liguori wrote:
>>
>>>> [1] JSON schema definition:
>>>>
>>>> { 'type': 'SnapshotDev',
>>>> 'data': {'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
>>>>
>>>> { 'command': 'blockdev-group-snapshot-sync',
>>>> 'data': { 'dev': [ 'SnapshotDev' ] } }
>>>
>>>
>>> This will end up looking like:
>>>
>>> { "execute": "blockdev-group-snapshot-sync",
>>> "arguments": { "dev": [{"device": "ide-hd0"....}] } }
>>
>> Does that mean we want it to look more like:
>>
>> { 'command': 'blockdev-group-snapshot-sync',
>> 'data': { 'devlist': [ 'SnapshotDev' ] } }
>>
>> and used like:
>>
>> { "execute": "blockdev-group-snapshot-sync",
>> "arguments": { "devlist": [{ "device": "ide-hd0" ...},
>> { "device": "ide-hd1" ...}] } }
>
>
> Yes, devlist is a better variable name.
>

I will change it to devlist, for the v2 patch submission.

>>
>> I don't quite care what syntax we use, as long as we settle on something
>> soon, because I'm trying to code up libvirt to generate this new monitor
>> command, and need to know what syntax it will use.
>
>
> Just FYI, I've looked at the QAPI bits so far. I would suggest getting
> an Ack from Kevin at least that this API is reasonable inline with what
> we're looking for here before you start doing libvirt support.
>
> Personally, I loathe the idea of extending an already broken
> interface... I would rather we fix the original interface first before
> tacking on additional features.
>
> Regards,
>
> Anthony Liguori
>
>

Re: [Qemu-devel] [PATCH 1/3] qapi: Allow QMP/QAPI commands to have array inputs

[Jeff Cody]

On 02/22/2012 12:35 PM, Anthony Liguori wrote:
> On 02/22/2012 10:12 AM, Jeff Cody wrote:
>> On 02/22/2012 09:53 AM, Anthony Liguori wrote:
>>> On 02/20/2012 11:31 AM, Jeff Cody wrote:
>>>> The QAPI scripts allow for generating commands that receive parameter
>>>> input consisting of a list of custom structs, but the QMP input
>>>> paramter
>>>> checking did not support receiving a qlist as opposed to a qdict for
>>>> input.
>>>
>>> What are you trying to send on the wire? Something like
>>>
>>> {"execute": "foo", "arguments": [ "a", "b", "c" ]}
>>>
>>> ?
>>>
>>> That's not valid per the QMP protocol. "arguments" must always be a QMP
>>> dictionary.
>>>
>>
>> Not a bare list like that (perhaps my wording was poor), but rather an
>> array of
>> custom structs, that contain QMP dicts. In this particular case:
>>
>> {"execute": "blockdev-group-snapshot-sync", "arguments": [{ "device":
>> "ide-hd0","snapshot-file":"/some/place/my-image","format": "qcow2" }]}
>>
>> When specifying this in the schema JSON file as shown below[1], the
>> QAPI scripts
>> generate the marshaller & visitor functions that handle everything
>> correctly.
>> The monitor code, however, could not deal with the QList container for
>> the input
>> parameters, prior to passing the data to the generated functions.
>>
>>
>> [1] JSON schema definition:
>>
>> { 'type': 'SnapshotDev',
>> 'data': {'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
>>
>> { 'command': 'blockdev-group-snapshot-sync',
>> 'data': { 'dev': [ 'SnapshotDev' ] } }
>
>
> This will end up looking like:
>
> { "execute": "blockdev-group-snapshot-sync",
> "arguments": { "dev": [{"device": "ide-hd0"....}] } }
>
> Which should work as expected in QMP. HMP argument validation doesn't
> handle arguments of lists but IMHO, it's time to kill that code.
>
> Luiz, what do you think?
>
> Regards,
>
> Anthony Liguori
>

You are right, I just tried it the way you suggested; that works fine. 
I will drop patch 1, and I will also be dropping patch 3.  Please ignore 
patch 1/3, thanks.




>>
>>
>>
>>>>
>>>> This patch allows for array input parameters, although no argument
>>>> validation
>>>> is performed for commands that accept a list input.
>>>>
>>>> Signed-off-by: Jeff Cody<jcody@redhat.com>
>>>> ---
>>>> monitor.c | 72
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++----------
>>>> monitor.h | 1 +
>>>> 2 files changed, 61 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/monitor.c b/monitor.c
>>>> index a7df995..98e6017 100644
>>>> --- a/monitor.c
>>>> +++ b/monitor.c
>>>> @@ -125,8 +125,12 @@ typedef struct mon_cmd_t {
>>>> void (*info)(Monitor *mon);
>>>> void (*cmd)(Monitor *mon, const QDict *qdict);
>>>> int (*cmd_new)(Monitor *mon, const QDict *params, QObject **ret_data);
>>>> + int (*cmd_new_list)(Monitor *mon, const QList *params,
>>>> + QObject **ret_data);
>>>> int (*cmd_async)(Monitor *mon, const QDict *params,
>>>> MonitorCompletion *cb, void *opaque);
>>>> + int (*cmd_async_list)(Monitor *mon, const QList *params,
>>>> + MonitorCompletion *cb, void *opaque);
>>>> } mhandler;
>>>> bool qapi;
>>>> int flags;
>>>> @@ -353,6 +357,11 @@ static inline bool handler_is_async(const
>>>> mon_cmd_t *cmd)
>>>> return cmd->flags& MONITOR_CMD_ASYNC;
>>>> }
>>>>
>>>> +static inline bool handler_accepts_array(const mon_cmd_t *cmd)
>>>> +{
>>>> + return cmd->flags& MONITOR_CMD_ARRAY_INPUT;
>>>> +}
>>>> +
>>>> static inline int monitor_has_error(const Monitor *mon)
>>>> {
>>>> return mon->error != NULL;
>>>> @@ -671,6 +680,12 @@ static int qmp_async_cmd_handler(Monitor *mon,
>>>> const mon_cmd_t *cmd,
>>>> return cmd->mhandler.cmd_async(mon, params, qmp_monitor_complete, mon);
>>>> }
>>>>
>>>> +static int qmp_async_cmd_handler_array(Monitor *mon, const mon_cmd_t
>>>> *cmd,
>>>> + const QList *params)
>>>> +{
>>>> + return cmd->mhandler.cmd_async_list(mon, params,
>>>> qmp_monitor_complete, mon);
>>>> +}
>>>> +
>>>> static void user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd,
>>>> const QDict *params)
>>>> {
>>>> @@ -4310,7 +4325,8 @@ static QDict *qmp_check_input_obj(QObject
>>>> *input_obj)
>>>> }
>>>> has_exec_key = 1;
>>>> } else if (!strcmp(arg_name, "arguments")) {
>>>> - if (qobject_type(arg_obj) != QTYPE_QDICT) {
>>>> + if ((qobject_type(arg_obj) != QTYPE_QDICT)&&
>>>> + (qobject_type(arg_obj) != QTYPE_QLIST)) {
>>>> qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "arguments",
>>>> "object");
>>>> return NULL;
>>>> @@ -4345,11 +4361,26 @@ static void qmp_call_cmd(Monitor *mon, const
>>>> mon_cmd_t *cmd,
>>>> qobject_decref(data);
>>>> }
>>>>
>>>> +static void qmp_call_cmd_array(Monitor *mon, const mon_cmd_t *cmd,
>>>> + const QList *params)
>>>> +{
>>>> + int ret;
>>>> + QObject *data = NULL;
>>>> +
>>>> + mon_print_count_init(mon);
>>>> +
>>>> + ret = cmd->mhandler.cmd_new_list(mon, params,&data);
>>>> + handler_audit(mon, cmd, ret);
>>>> + monitor_protocol_emitter(mon, data);
>>>> + qobject_decref(data);
>>>> +}
>>>> +
>>>> static void handle_qmp_command(JSONMessageParser *parser, QList
>>>> *tokens)
>>>> {
>>>> int err;
>>>> QObject *obj;
>>>> QDict *input, *args;
>>>> + QList *args_list = NULL;
>>>> const mon_cmd_t *cmd;
>>>> const char *cmd_name;
>>>> Monitor *mon = cur_mon;
>>>> @@ -4386,26 +4417,42 @@ static void
>>>> handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>>>> }
>>>>
>>>> obj = qdict_get(input, "arguments");
>>>> - if (!obj) {
>>>> - args = qdict_new();
>>>> + if (handler_accepts_array(cmd)) {
>>>> + if (!obj || (qobject_type(obj) != QTYPE_QLIST)) {
>>>> + args_list = qlist_new();
>>>> + } else {
>>>> + args_list = qobject_to_qlist(obj);
>>>> + QINCREF(args_list);
>>>> + }
>>>> } else {
>>>> - args = qobject_to_qdict(obj);
>>>> - QINCREF(args);
>>>> - }
>>>> -
>>>> - err = qmp_check_client_args(cmd, args);
>>>> - if (err< 0) {
>>>> - goto err_out;
>>>> + if (!obj || (qobject_type(obj) != QTYPE_QDICT)) {
>>>> + args = qdict_new();
>>>> + } else {
>>>> + args = qobject_to_qdict(obj);
>>>> + QINCREF(args);
>>>> + }
>>>> + err = qmp_check_client_args(cmd, args);
>>>> + if (err< 0) {
>>>> + goto err_out;
>>>> + }
>>>> }
>>>>
>>>> if (handler_is_async(cmd)) {
>>>> - err = qmp_async_cmd_handler(mon, cmd, args);
>>>> + if (handler_accepts_array(cmd)) {
>>>> + err = qmp_async_cmd_handler_array(mon, cmd, args_list);
>>>> + } else {
>>>> + err = qmp_async_cmd_handler(mon, cmd, args);
>>>> + }
>>>> if (err) {
>>>> /* emit the error response */
>>>> goto err_out;
>>>> }
>>>> } else {
>>>> - qmp_call_cmd(mon, cmd, args);
>>>> + if (handler_accepts_array(cmd)) {
>>>> + qmp_call_cmd_array(mon, cmd, args_list);
>>>> + } else {
>>>> + qmp_call_cmd(mon, cmd, args);
>>>> + }
>>>> }
>>>>
>>>> goto out;
>>>> @@ -4415,6 +4462,7 @@ err_out:
>>>> out:
>>>> QDECREF(input);
>>>> QDECREF(args);
>>>> + QDECREF(args_list);
>>>> }
>>>>
>>>> /**
>>>> diff --git a/monitor.h b/monitor.h
>>>> index b72ea07..499fc1f 100644
>>>> --- a/monitor.h
>>>> +++ b/monitor.h
>>>> @@ -19,6 +19,7 @@ extern Monitor *default_mon;
>>>>
>>>> /* flags for monitor commands */
>>>> #define MONITOR_CMD_ASYNC 0x0001
>>>> +#define MONITOR_CMD_ARRAY_INPUT 0x0002
>>>>
>>>> /* QMP events */
>>>> typedef enum MonitorEvent {
>>>
>>
>
>

Re: [Qemu-devel] [PATCH 1/3] qapi: Allow QMP/QAPI commands to have array inputs

[Luiz Capitulino]

On Wed, 22 Feb 2012 11:35:26 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 02/22/2012 10:12 AM, Jeff Cody wrote:
> > On 02/22/2012 09:53 AM, Anthony Liguori wrote:
> >> On 02/20/2012 11:31 AM, Jeff Cody wrote:
> >>> The QAPI scripts allow for generating commands that receive parameter
> >>> input consisting of a list of custom structs, but the QMP input paramter
> >>> checking did not support receiving a qlist as opposed to a qdict for
> >>> input.
> >>
> >> What are you trying to send on the wire? Something like
> >>
> >> {"execute": "foo", "arguments": [ "a", "b", "c" ]}
> >>
> >> ?
> >>
> >> That's not valid per the QMP protocol. "arguments" must always be a QMP
> >> dictionary.
> >>
> >
> > Not a bare list like that (perhaps my wording was poor), but rather an array of
> > custom structs, that contain QMP dicts. In this particular case:
> >
> > {"execute": "blockdev-group-snapshot-sync", "arguments": [{ "device":
> > "ide-hd0","snapshot-file":"/some/place/my-image","format": "qcow2" }]}
> >
> > When specifying this in the schema JSON file as shown below[1], the QAPI scripts
> > generate the marshaller & visitor functions that handle everything correctly.
> > The monitor code, however, could not deal with the QList container for the input
> > parameters, prior to passing the data to the generated functions.
> >
> >
> > [1] JSON schema definition:
> >
> > { 'type': 'SnapshotDev',
> > 'data': {'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
> >
> > { 'command': 'blockdev-group-snapshot-sync',
> > 'data': { 'dev': [ 'SnapshotDev' ] } }
> 
> 
> This will end up looking like:
> 
> { "execute": "blockdev-group-snapshot-sync",
>    "arguments": { "dev": [{"device": "ide-hd0"....}] } }
> 
> Which should work as expected in QMP.  HMP argument validation doesn't handle 
> arguments of lists but IMHO, it's time to kill that code.
> 
> Luiz, what do you think?

I don't think the code can just be dropped (if that's what you meant), as there
are some features on top of it, like auto-completion, suffix support (G, M, k, etc),
but it certainly can be improved.

Also note that QMP does argument validation too, which is unfortunate enough to
be based on HMP's, and it probably has to be adapted too (actually, maybe you can
use the O type for lists...).

Re: [Qemu-devel] [PATCH 1/3] qapi: Allow QMP/QAPI commands to have array inputs

[Anthony Liguori]

On 02/22/2012 02:25 PM, Luiz Capitulino wrote:
> On Wed, 22 Feb 2012 11:35:26 -0600
> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>
>> On 02/22/2012 10:12 AM, Jeff Cody wrote:
>>> On 02/22/2012 09:53 AM, Anthony Liguori wrote:
>>>> On 02/20/2012 11:31 AM, Jeff Cody wrote:
>>>>> The QAPI scripts allow for generating commands that receive parameter
>>>>> input consisting of a list of custom structs, but the QMP input paramter
>>>>> checking did not support receiving a qlist as opposed to a qdict for
>>>>> input.
>>>>
>>>> What are you trying to send on the wire? Something like
>>>>
>>>> {"execute": "foo", "arguments": [ "a", "b", "c" ]}
>>>>
>>>> ?
>>>>
>>>> That's not valid per the QMP protocol. "arguments" must always be a QMP
>>>> dictionary.
>>>>
>>>
>>> Not a bare list like that (perhaps my wording was poor), but rather an array of
>>> custom structs, that contain QMP dicts. In this particular case:
>>>
>>> {"execute": "blockdev-group-snapshot-sync", "arguments": [{ "device":
>>> "ide-hd0","snapshot-file":"/some/place/my-image","format": "qcow2" }]}
>>>
>>> When specifying this in the schema JSON file as shown below[1], the QAPI scripts
>>> generate the marshaller&  visitor functions that handle everything correctly.
>>> The monitor code, however, could not deal with the QList container for the input
>>> parameters, prior to passing the data to the generated functions.
>>>
>>>
>>> [1] JSON schema definition:
>>>
>>> { 'type': 'SnapshotDev',
>>> 'data': {'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
>>>
>>> { 'command': 'blockdev-group-snapshot-sync',
>>> 'data': { 'dev': [ 'SnapshotDev' ] } }
>>
>>
>> This will end up looking like:
>>
>> { "execute": "blockdev-group-snapshot-sync",
>>     "arguments": { "dev": [{"device": "ide-hd0"....}] } }
>>
>> Which should work as expected in QMP.  HMP argument validation doesn't handle
>> arguments of lists but IMHO, it's time to kill that code.
>>
>> Luiz, what do you think?
>
> I don't think the code can just be dropped (if that's what you meant), as there
> are some features on top of it, like auto-completion, suffix support (G, M, k, etc),
> but it certainly can be improved.
>
> Also note that QMP does argument validation too, which is unfortunate enough to
> be based on HMP's,

Once everything is converted to middle mode, this validation can be dropped for 
QMP.  The generated code should do thorough checking of arguments and can handle 
complex types.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/3] qapi: Allow QMP/QAPI commands to have array inputs

[Luiz Capitulino]

On Wed, 22 Feb 2012 14:31:17 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 02/22/2012 02:25 PM, Luiz Capitulino wrote:
> > On Wed, 22 Feb 2012 11:35:26 -0600
> > Anthony Liguori<anthony@codemonkey.ws>  wrote:
> >
> >> On 02/22/2012 10:12 AM, Jeff Cody wrote:
> >>> On 02/22/2012 09:53 AM, Anthony Liguori wrote:
> >>>> On 02/20/2012 11:31 AM, Jeff Cody wrote:
> >>>>> The QAPI scripts allow for generating commands that receive parameter
> >>>>> input consisting of a list of custom structs, but the QMP input paramter
> >>>>> checking did not support receiving a qlist as opposed to a qdict for
> >>>>> input.
> >>>>
> >>>> What are you trying to send on the wire? Something like
> >>>>
> >>>> {"execute": "foo", "arguments": [ "a", "b", "c" ]}
> >>>>
> >>>> ?
> >>>>
> >>>> That's not valid per the QMP protocol. "arguments" must always be a QMP
> >>>> dictionary.
> >>>>
> >>>
> >>> Not a bare list like that (perhaps my wording was poor), but rather an array of
> >>> custom structs, that contain QMP dicts. In this particular case:
> >>>
> >>> {"execute": "blockdev-group-snapshot-sync", "arguments": [{ "device":
> >>> "ide-hd0","snapshot-file":"/some/place/my-image","format": "qcow2" }]}
> >>>
> >>> When specifying this in the schema JSON file as shown below[1], the QAPI scripts
> >>> generate the marshaller&  visitor functions that handle everything correctly.
> >>> The monitor code, however, could not deal with the QList container for the input
> >>> parameters, prior to passing the data to the generated functions.
> >>>
> >>>
> >>> [1] JSON schema definition:
> >>>
> >>> { 'type': 'SnapshotDev',
> >>> 'data': {'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
> >>>
> >>> { 'command': 'blockdev-group-snapshot-sync',
> >>> 'data': { 'dev': [ 'SnapshotDev' ] } }
> >>
> >>
> >> This will end up looking like:
> >>
> >> { "execute": "blockdev-group-snapshot-sync",
> >>     "arguments": { "dev": [{"device": "ide-hd0"....}] } }
> >>
> >> Which should work as expected in QMP.  HMP argument validation doesn't handle
> >> arguments of lists but IMHO, it's time to kill that code.
> >>
> >> Luiz, what do you think?
> >
> > I don't think the code can just be dropped (if that's what you meant), as there
> > are some features on top of it, like auto-completion, suffix support (G, M, k, etc),
> > but it certainly can be improved.
> >
> > Also note that QMP does argument validation too, which is unfortunate enough to
> > be based on HMP's,
> 
> Once everything is converted to middle mode, this validation can be dropped for 
> QMP.  The generated code should do thorough checking of arguments and can handle 
> complex types.

That's cool, although we'll be switching to the new server too (hence the whole
current server will be dropped).

[Qemu-devel] [PATCH 2/3] qapi: Introduce blockdev-group-snapshot-sync command

[Jeff Cody]

This is a QAPI/QMP only command to take a snapshot of a group of
devices. This is simlar to the blockdev-snapshot-sync command, except
blockdev-group-snapshot-sync accepts a list devices, filenames, and
formats.

It is attempted to keep the snapshot of the group atomic; if
any snapshot of a device in a given group fails, then the whole group
is reverted back to its original image, and error is reported.

This allows the group of disks to remain consistent with each other,
even across snapshot failures.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 blockdev.c       |  130 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json |   45 +++++++++++++++++++
 qmp-commands.hx  |   39 ++++++++++++++++
 3 files changed, 214 insertions(+), 0 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 05e7c5e..0149720 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -714,6 +714,136 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
     }
 }
 
+
+typedef struct BlkGroupSnapshotData {
+    BlockDriverState *bs;
+    BlockDriver *drv;
+    BlockDriver *old_drv;
+    int flags;
+    const char *format;
+    char old_filename[1024];
+    const char *snapshot_file;
+    bool has_pivoted;
+    QSIMPLEQ_ENTRY(BlkGroupSnapshotData) entry;
+} BlkGroupSnapshotData;
+
+/*
+ * 'Atomic' group snapshots.  The snapshots are taken as a set, and if any fail
+ *  then we attempt to undo all of the pivots performed.
+ */
+void qmp_blockdev_group_snapshot_sync(SnapshotDevList *dev_list,
+                                                   Error **errp)
+{
+    int ret = 0;
+    SnapshotDevList *dev_entry = dev_list;
+    SnapshotDev *dev_info = NULL;
+    BlkGroupSnapshotData *snap_entry;
+    BlockDriver *proto_drv;
+
+    QSIMPLEQ_HEAD(gsnp_list, BlkGroupSnapshotData) gsnp_list;
+    QSIMPLEQ_INIT(&gsnp_list);
+
+    /* We don't do anything in this loop that commits us to the snapshot */
+    while (NULL != dev_entry) {
+        dev_info = dev_entry->value;
+        dev_entry = dev_entry->next;
+
+        snap_entry = g_malloc0(sizeof(BlkGroupSnapshotData));
+
+        snap_entry->bs = bdrv_find(dev_info->device);
+
+        if (!snap_entry->bs) {
+            error_set(errp, QERR_DEVICE_NOT_FOUND, dev_info->device);
+            goto exit;
+        }
+        if (bdrv_in_use(snap_entry->bs)) {
+            error_set(errp, QERR_DEVICE_IN_USE, dev_info->device);
+            goto exit;
+        }
+
+        pstrcpy(snap_entry->old_filename, sizeof(snap_entry->old_filename),
+                snap_entry->bs->filename);
+
+        snap_entry->snapshot_file = dev_info->snapshot_file;
+        snap_entry->old_drv = snap_entry->bs->drv;
+        snap_entry->flags = snap_entry->bs->open_flags;
+
+        if (!dev_info->has_format) {
+            snap_entry->format = "qcow2";
+        } else {
+            snap_entry->format = dev_info->format;
+        }
+
+        snap_entry->drv = bdrv_find_format(snap_entry->format);
+        if (!snap_entry->drv) {
+            error_set(errp, QERR_INVALID_BLOCK_FORMAT, snap_entry->format);
+            goto exit;
+        }
+
+        proto_drv = bdrv_find_protocol(snap_entry->snapshot_file);
+        if (!proto_drv) {
+            error_set(errp, QERR_INVALID_BLOCK_FORMAT, snap_entry->format);
+            goto exit;
+        }
+        ret = bdrv_img_create(snap_entry->snapshot_file, snap_entry->format,
+                              snap_entry->bs->filename,
+                              snap_entry->bs->drv->format_name, NULL, -1,
+                              snap_entry->flags);
+        if (ret) {
+            error_set(errp, QERR_UNDEFINED_ERROR);
+            goto exit;
+        }
+        snap_entry->has_pivoted = false;
+        QSIMPLEQ_INSERT_TAIL(&gsnp_list, snap_entry, entry);
+    }
+
+    /*
+     * Now we will drain, flush, & pivot everything - if any of the pivots fail,
+     * we will flag an error, and attempt to rollback.
+     */
+    bdrv_drain_all();
+    QSIMPLEQ_FOREACH(snap_entry, &gsnp_list, entry) {
+        bdrv_flush(snap_entry->bs);
+        bdrv_close(snap_entry->bs);
+
+        ret = bdrv_open(snap_entry->bs, snap_entry->snapshot_file,
+                        snap_entry->flags, snap_entry->drv);
+        snap_entry->has_pivoted = true;
+        /*
+         * If we fail any of these, then we need to rollback all that we have
+         * performed up to this point
+         */
+        if (ret != 0) {
+            error_set(errp, QERR_OPEN_FILE_FAILED, snap_entry->snapshot_file);
+            goto error_rollback;
+        }
+    }
+
+    /* success */
+    goto exit;
+
+error_rollback:
+    /* failure, undo everything as much as we can */
+    QSIMPLEQ_FOREACH(snap_entry, &gsnp_list, entry) {
+        if (snap_entry->has_pivoted) {
+            ret = bdrv_open(snap_entry->bs, snap_entry->old_filename,
+                            snap_entry->flags, snap_entry->old_drv);
+            if (ret != 0) {
+                /* This is very very bad */
+                error_set(errp, QERR_OPEN_FILE_FAILED,
+                          snap_entry->old_filename);
+            }
+        }
+    }
+exit:
+    QSIMPLEQ_FOREACH(snap_entry, &gsnp_list, entry) {
+        g_free(snap_entry);
+    }
+
+    return;
+}
+
+
 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 80debe6..b8d66d0 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1107,6 +1107,51 @@
 { 'command': 'block_resize', 'data': { 'device': 'str', 'size': 'int' }}
 
 ##
+# @SnapshotDev
+#
+# @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.
+#
+# @format: #optional the format of the snapshot image, default is 'qcow2'.
+##
+{ 'type': 'SnapshotDev',
+  'data': {'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
+
+##
+# @blockdev-group-snapshot-sync
+#
+# Generates a synchronous snapshot of a group of one or more block devices,
+# as atomically as possible.
+#
+#  List of:
+#  @SnapshotDev: information needed for the device snapshot
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If @snapshot-file can't be opened, OpenFileFailed
+#          If @format is invalid, InvalidBlockFormat
+#
+# Notes: One of the last steps taken by this command is to close the current
+#        images being used by each specified @device and open the corresponding
+#        @snapshot-file one. If that fails, the command will try to reopen
+#        all of the original image files, for each device specified. If
+#        that also fails OpenFileFailed will be returned and the guest may get
+#        unexpected errors.
+#
+#        Since there could potentially be more than one file open or drive
+#        failures, the additional command 'blockdev-query-group-snapshot-failure'
+#        will return a list of all device files that have failed.  This could
+#        include the original filename if the reopen of an original image file
+#        failed.
+#
+##
+{ 'command': 'blockdev-group-snapshot-sync',
+  'data': { 'dev': [ 'SnapshotDev' ] } }
+
+##
 # @blockdev-snapshot-sync
 #
 # Generates a synchronous snapshot of a block device.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index bd6b641..2fe1e6e 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -665,6 +665,45 @@ EQMP
         .args_type  = "device:B",
         .mhandler.cmd_new = qmp_marshal_input_block_job_cancel,
     },
+    {
+        .name       = "blockdev-group-snapshot-sync",
+        .args_type  = "device:B,snapshot-file:s,format:s?",
+        .mhandler.cmd_new = qmp_marshal_input_blockdev_group_snapshot_sync,
+        .flags      = MONITOR_CMD_ARRAY_INPUT,
+    },
+
+SQMP
+blockdev-group-snapshot-sync
+----------------------
+
+Synchronous snapshot of one or more block devices.  A list array input
+is accepted, that contains the device, snapshot-file to be create as 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. format specifies the format of the
+snapshot image, default is qcow2.  On failure of any device, it is
+attempted to reopen the original image for all the devices that were
+specified.
+
+Arguments:
+
+- "device": device name to snapshot (json-string)
+- "snapshot-file": name of new image file (json-string)
+- "format": format of new image (json-string, optional)
+
+Example:
+
+-> { "execute": "blockdev-group-snapshot-sync", "arguments": [{ "device": "ide-hd0",
+                                                                "snapshot-file":
+                                                                "/some/place/my-image",
+                                                                "format": "qcow2" },
+                                                              { "device": "ide-hd1",
+                                                                "snapshot-file":
+                                                                "/some/place/my-image2",
+                                                                "format": "qcow2" } }
+<- { "return": {} }
+
+EQMP
 
     {
         .name       = "blockdev-snapshot-sync",
-- 
1.7.9.rc2.1.g69204

Re: [Qemu-devel] [PATCH 2/3] qapi: Introduce blockdev-group-snapshot-sync command

[Eric Blake]

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

On 02/20/2012 10:31 AM, Jeff Cody wrote:
> This is a QAPI/QMP only command to take a snapshot of a group of
> devices. This is simlar to the blockdev-snapshot-sync command, except

s/simlar/similar/

> blockdev-group-snapshot-sync accepts a list devices, filenames, and
> formats.
> 
> It is attempted to keep the snapshot of the group atomic; if
> any snapshot of a device in a given group fails, then the whole group
> is reverted back to its original image, and error is reported.
> 
> This allows the group of disks to remain consistent with each other,
> even across snapshot failures.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  blockdev.c       |  130 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json |   45 +++++++++++++++++++
>  qmp-commands.hx  |   39 ++++++++++++++++
>  3 files changed, 214 insertions(+), 0 deletions(-)

> +
> +error_rollback:
> +    /* failure, undo everything as much as we can */
> +    QSIMPLEQ_FOREACH(snap_entry, &gsnp_list, entry) {
> +        if (snap_entry->has_pivoted) {
> +            ret = bdrv_open(snap_entry->bs, snap_entry->old_filename,
> +                            snap_entry->flags, snap_entry->old_drv);
> +            if (ret != 0) {
> +                /* This is very very bad */
> +                error_set(errp, QERR_OPEN_FILE_FAILED,
> +                          snap_entry->old_filename);

Is there any way to reduce the likelihood of a rollback failure?


> +SQMP
> +blockdev-group-snapshot-sync
> +----------------------
> +
> +Synchronous snapshot of one or more block devices.  A list array input
> +is accepted, that contains the device, snapshot-file to be create as 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. format specifies the format of the
> +snapshot image, default is qcow2.  On failure of any device, it is
> +attempted to reopen the original image for all the devices that were
> +specified.
> +
> +Arguments:
> +
> +- "device": device name to snapshot (json-string)
> +- "snapshot-file": name of new image file (json-string)
> +- "format": format of new image (json-string, optional)

Shouldn't this mention that the arguments is a JSON list, rather than a
single argument?

> +
> +Example:
> +
> +-> { "execute": "blockdev-group-snapshot-sync", "arguments": [{ "device": "ide-hd0",
> +                                                                "snapshot-file":
> +                                                                "/some/place/my-image",
> +                                                                "format": "qcow2" },
> +                                                              { "device": "ide-hd1",
> +                                                                "snapshot-file":
> +                                                                "/some/place/my-image2",
> +                                                                "format": "qcow2" } }

Are you missing a ']' before the final '}' here?

-- 
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 2/3] qapi: Introduce blockdev-group-snapshot-sync command

[Jeff Cody]

On 02/20/2012 12:41 PM, Eric Blake wrote:
> On 02/20/2012 10:31 AM, Jeff Cody wrote:
>> This is a QAPI/QMP only command to take a snapshot of a group of
>> devices. This is simlar to the blockdev-snapshot-sync command, except
>
> s/simlar/similar/
>

Oops - fixed for v2.


>> blockdev-group-snapshot-sync accepts a list devices, filenames, and
>> formats.
>>
>> It is attempted to keep the snapshot of the group atomic; if
>> any snapshot of a device in a given group fails, then the whole group
>> is reverted back to its original image, and error is reported.
>>
>> This allows the group of disks to remain consistent with each other,
>> even across snapshot failures.
>>
>> Signed-off-by: Jeff Cody<jcody@redhat.com>
>> ---
>>   blockdev.c       |  130 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   qapi-schema.json |   45 +++++++++++++++++++
>>   qmp-commands.hx  |   39 ++++++++++++++++
>>   3 files changed, 214 insertions(+), 0 deletions(-)
>
>> +
>> +error_rollback:
>> +    /* failure, undo everything as much as we can */
>> +    QSIMPLEQ_FOREACH(snap_entry,&gsnp_list, entry) {
>> +        if (snap_entry->has_pivoted) {
>> +            ret = bdrv_open(snap_entry->bs, snap_entry->old_filename,
>> +                            snap_entry->flags, snap_entry->old_drv);
>> +            if (ret != 0) {
>> +                /* This is very very bad */
>> +                error_set(errp, QERR_OPEN_FILE_FAILED,
>> +                          snap_entry->old_filename);
>
> Is there any way to reduce the likelihood of a rollback failure?

Good question.  Obviously, it is ideal for this to have the lowest 
possible likelihood of a rollback failure.  I guess the real question 
is: What are the events that would cause the original image re-open to 
fail, and how can we avoid them - or, as Kevin mentioned to me, do we 
never close the original file to avoid the re-open (and, will that avoid 
those failure events)?  Hopefully these are rare events, in any case.

>
>
>> +SQMP
>> +blockdev-group-snapshot-sync
>> +----------------------
>> +
>> +Synchronous snapshot of one or more block devices.  A list array input
>> +is accepted, that contains the device, snapshot-file to be create as 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. format specifies the format of the
>> +snapshot image, default is qcow2.  On failure of any device, it is
>> +attempted to reopen the original image for all the devices that were
>> +specified.
>> +
>> +Arguments:
>> +
>> +- "device": device name to snapshot (json-string)
>> +- "snapshot-file": name of new image file (json-string)
>> +- "format": format of new image (json-string, optional)
>
> Shouldn't this mention that the arguments is a JSON list, rather than a
> single argument?

Yes, you are right - adding that in.

>
>> +
>> +Example:
>> +
>> +->  { "execute": "blockdev-group-snapshot-sync", "arguments": [{ "device": "ide-hd0",
>> +                                                                "snapshot-file":
>> +                                                                "/some/place/my-image",
>> +                                                                "format": "qcow2" },
>> +                                                              { "device": "ide-hd1",
>> +                                                                "snapshot-file":
>> +                                                                "/some/place/my-image2",
>> +                                                                "format": "qcow2" } }
>
> Are you missing a ']' before the final '}' here?
>

Yes, thanks.  Forgot that in the example.  Added for v2.

Jeff

[Qemu-devel] [PATCH 3/3] qapi: Introduce blockdev-query-group-snapshot-failure

[Jeff Cody]

In the case of a failure in a group snapshot, it is possible for
multiple file image failures to occur - for instance, failure of
an original snapshot, and then failure of one or more of the
attempted reopens of the original.

Knowing all of the file images which failed could be useful or
critical information, so this command returns a list of strings
containing the filenames of all failures from the last
invocation of blockdev-group-snapshot-sync.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 blockdev.c       |   42 +++++++++++++++++++++++++++++++++++++++++-
 qapi-schema.json |   25 +++++++++++++++++++++++++
 qmp-commands.hx  |    5 +++++
 3 files changed, 71 insertions(+), 1 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 0149720..cb44af5 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -727,6 +727,7 @@ typedef struct BlkGroupSnapshotData {
     QSIMPLEQ_ENTRY(BlkGroupSnapshotData) entry;
 } BlkGroupSnapshotData;
 
+SnapshotFailList *group_snap_fail_list;
 /*
  * 'Atomic' group snapshots.  The snapshots are taken as a set, and if any fail
  *  then we attempt to undo all of the pivots performed.
@@ -739,10 +740,24 @@ void qmp_blockdev_group_snapshot_sync(SnapshotDevList *dev_list,
     SnapshotDev *dev_info = NULL;
     BlkGroupSnapshotData *snap_entry;
     BlockDriver *proto_drv;
+    SnapshotFailList *fail_entry = group_snap_fail_list;
 
     QSIMPLEQ_HEAD(gsnp_list, BlkGroupSnapshotData) gsnp_list;
     QSIMPLEQ_INIT(&gsnp_list);
 
+    /*
+     * clear out our failure list first, and reclaim memory
+     * we maintain the list, so if a group snapshot fails
+     * we can be queried about which devices failed
+     */
+    SnapshotFailList *fail_entry_next = NULL;
+    while (NULL != fail_entry) {
+        g_free(fail_entry->value);
+        fail_entry_next = fail_entry->next;
+        g_free(fail_entry);
+        fail_entry = fail_entry_next;
+    }
+
     /* We don't do anything in this loop that commits us to the snapshot */
     while (NULL != dev_entry) {
         dev_info = dev_entry->value;
@@ -815,6 +830,16 @@ void qmp_blockdev_group_snapshot_sync(SnapshotDevList *dev_list,
          */
         if (ret != 0) {
             error_set(errp, QERR_OPEN_FILE_FAILED, snap_entry->snapshot_file);
+            /*
+             * We bail on the first failure, but add the failed filename to the
+             * return list in case any of the rollback pivots fail as well
+             */
+            SnapshotFailList *failure;
+            failure = g_malloc0(sizeof(SnapshotFailList));
+            failure->value = g_malloc0(sizeof(*failure->value));
+            failure->value->failed_file = g_strdup(snap_entry->snapshot_file);
+            failure->next = group_snap_fail_list;
+            group_snap_fail_list = failure;
             goto error_rollback;
         }
     }
@@ -829,7 +854,17 @@ error_rollback:
             ret = bdrv_open(snap_entry->bs, snap_entry->old_filename,
                             snap_entry->flags, snap_entry->old_drv);
             if (ret != 0) {
-                /* This is very very bad */
+                /*
+                 * This is very very bad.  Make sure the caller is aware
+                 * of which files failed, since there could be more than
+                 * one
+                 */
+                SnapshotFailList *failure;
+                failure = g_malloc0(sizeof(SnapshotFailList));
+                failure->value = g_malloc0(sizeof(*failure->value));
+                failure->value->failed_file = g_strdup(snap_entry->old_filename);
+                failure->next = group_snap_fail_list;
+                group_snap_fail_list = failure;
                 error_set(errp, QERR_OPEN_FILE_FAILED,
                           snap_entry->old_filename);
             }
@@ -843,6 +878,11 @@ exit:
     return;
 }
 
+SnapshotFailList *qmp_blockdev_query_group_snapshot_failure(Error **errp)
+{
+    return group_snap_fail_list;
+}
+
 
 static void eject_device(BlockDriverState *bs, int force, Error **errp)
 {
diff --git a/qapi-schema.json b/qapi-schema.json
index b8d66d0..c4b27a3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1121,6 +1121,14 @@
   'data': {'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
 
 ##
+# @SnapshotFail
+#
+#  @failed_file:	string of the filename that failed
+##
+{ 'type': 'SnapshotFail',
+  'data': {'failed_file': 'str' } }
+
+##
 # @blockdev-group-snapshot-sync
 #
 # Generates a synchronous snapshot of a group of one or more block devices,
@@ -1152,6 +1160,23 @@
   'data': { 'dev': [ 'SnapshotDev' ] } }
 
 ##
+# @blockdev-query-group-snapshot-failure
+#
+#
+# Returns: A list of @SnapshotFail, that contains the filenames for all failures
+#		   of the last blockdev-group-snapshot-sync command.
+#
+# Notes:
+#        Since there could potentially be more than one file open or drive
+#        failures, the additional command 'blockdev-query-group-snapshot-failure'
+#        will return a list of all device files that have failed.  This could
+#        include the original filename if the reopen of an original image file
+#        failed.
+#
+##
+{ 'command': 'blockdev-query-group-snapshot-failure', 'returns': [ 'SnapshotFail' ] }
+
+##
 # @blockdev-snapshot-sync
 #
 # Generates a synchronous snapshot of a block device.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 2fe1e6e..9a80e08 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -706,6 +706,11 @@ Example:
 EQMP
 
     {
+        .name       = "blockdev-query-group-snapshot-failure",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_blockdev_query_group_snapshot_failure,
+    },
+    {
         .name       = "blockdev-snapshot-sync",
         .args_type  = "device:B,snapshot-file:s,format:s?",
         .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,
-- 
1.7.9.rc2.1.g69204

Re: [Qemu-devel] [PATCH 3/3] qapi: Introduce blockdev-query-group-snapshot-failure

[Eric Blake]

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

On 02/20/2012 10:31 AM, Jeff Cody wrote:
> In the case of a failure in a group snapshot, it is possible for
> multiple file image failures to occur - for instance, failure of
> an original snapshot, and then failure of one or more of the
> attempted reopens of the original.
> 
> Knowing all of the file images which failed could be useful or
> critical information, so this command returns a list of strings
> containing the filenames of all failures from the last
> invocation of blockdev-group-snapshot-sync.

Meta-question:

Suppose that the guest is running when we issue
blockdev-group-snapshot-sync - in that case, qemu is responsible for
pausing and then resuming the guest.  On success, this makes sense.  But
what happens on failure?

If we only fail at creating one snapshot, but successfully roll back the
rest of the set, should the guest be resumed (as if the command had
never been attempted), or should the guest be left paused?

On the other hand, if we fail at creating one snapshot, as well as fail
at rolling back, then that argues that we _cannot_ resume the guest,
because we no longer have a block device open.

This policy needs to be documented in one (or both) of the two new
monitor commands, and we probably ought to make sure that if the guest
is left paused where it had originally started as running, then an
appropriate event is also emitted.

For blockdev-snapshot-sync, libvirt was always pausing qemu before
issuing the snapshot, then resuming afterwards; but now that we have the
ability to make the set atomic, I'm debating about whether libvirt still
needs to pause qemu, or whether it can now rely on qemu doing the right
things about pausing and resuming as part of the snapshot command.

-- 
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 3/3] qapi: Introduce blockdev-query-group-snapshot-failure

[Jeff Cody]

On 02/20/2012 12:48 PM, Eric Blake wrote:
> On 02/20/2012 10:31 AM, Jeff Cody wrote:
>> In the case of a failure in a group snapshot, it is possible for
>> multiple file image failures to occur - for instance, failure of
>> an original snapshot, and then failure of one or more of the
>> attempted reopens of the original.
>>
>> Knowing all of the file images which failed could be useful or
>> critical information, so this command returns a list of strings
>> containing the filenames of all failures from the last
>> invocation of blockdev-group-snapshot-sync.
> 
> Meta-question:
> 
> Suppose that the guest is running when we issue
> blockdev-group-snapshot-sync - in that case, qemu is responsible for
> pausing and then resuming the guest.  On success, this makes sense.  But
> what happens on failure?

The guest is not paused in blockdev-group-snapshot-sync; I don't think
that qemu should enforce pause/resume in the live snapshot commands.

> 
> If we only fail at creating one snapshot, but successfully roll back the
> rest of the set, should the guest be resumed (as if the command had
> never been attempted), or should the guest be left paused?
> 
> On the other hand, if we fail at creating one snapshot, as well as fail
> at rolling back, then that argues that we _cannot_ resume the guest,
> because we no longer have a block device open.

Is that really true, though?  Depending on what drive failed, the guest
may still be runnable.  It would be roughly equivalent to the guest as a
drive failure; a bad event, but not always fatal.

But, I think v2 of the patch may make this moot - I was talking with
Kevin, and he had some good ideas on how to do this without requiring a
close & reopen in the case of the snapshot failure; which means that we
shouldn't have to worry about the second scenario.  I am going to
incorporate those changes into v2.

> 
> This policy needs to be documented in one (or both) of the two new
> monitor commands, and we probably ought to make sure that if the guest
> is left paused where it had originally started as running, then an
> appropriate event is also emitted.

I agree, the documentation should make it clear what is going on - I
will add that to v2.

> 
> For blockdev-snapshot-sync, libvirt was always pausing qemu before
> issuing the snapshot, then resuming afterwards; but now that we have the
> ability to make the set atomic, I'm debating about whether libvirt still
> needs to pause qemu, or whether it can now rely on qemu doing the right
> things about pausing and resuming as part of the snapshot command.
> 

Again, it doesn't pause automatically, so that is up to libvirt.  The
guest agent is also available to freeze the filesystem, if libvirt wants
to trust it (and it is running); if not, then libvirt can still issue a
pause/resume around the snapshot command (and libvirt may be in a better
position to decide what to do in case of failure, if it has some
knowledge of the drives that failed and how they are used).