Group Live Snapshots

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.

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 <[email protected]>

---
 blockdev.c       |  130 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json |   45 +++++++++++++++++++
 qmp-commands.hx  |   39 ++++++++++++++++
 3 files changed, 214 insertions(+), 0 deletions(-)

blockdev.c +1 -0

+

qmp-commands.hx +8 -0

+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

Quoted 2 lines · 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

s/simlar/similar/

Quoted 16 lines

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<[email protected]>

---
  blockdev.c       |  130 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
  qapi-schema.json |   45 +++++++++++++++++++
  qmp-commands.hx  |   39 ++++++++++++++++
  3 files changed, 214 insertions(+), 0 deletions(-)

blockdev.c void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file, · line 834

Quoted 11 lines

blockdev.c +11 -0

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

Quoted 15 lines

qmp-commands.hx +13 -0

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

qmp-commands.hx EQMP · line 692

Quoted 5 lines

qmp-commands.hx +5 -0

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

qmp-commands.hx EQMP · line 703

Quoted 12 lines

qmp-commands.hx +11 -0

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

Quoted 9 lines · 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.

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.

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<[email protected]>

---
  monitor.c |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++----------
  monitor.h |    1 +
  2 files changed, 61 insertions(+), 12 deletions(-)

monitor.c +60 -12

index a7df995..98e6017 100644
@@ -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);
  }
 
  /**

monitor.h +1 -0

index b72ea07..499fc1f 100644
@@ -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 {

Quoted 4 lines · 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.

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.

Quoted 172 lines

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<[email protected]>

---
monitor.c | 72
++++++++++++++++++++++++++++++++++++++++++++++++++----------
monitor.h | 1 +

2 files changed, 61 insertions(+), 12 deletions(-)

monitor.c +9 -0

index a7df995..98e6017 100644
@@ -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;

monitor.c +6 -0

@@ -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)
{

monitor.c +2 -1

@@ -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;

monitor.c +15 -0

@@ -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;

monitor.c +27 -11

@@ -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;

monitor.c +0 -0

@@ -4415,6 +4462,7 @@ err_out:

out:
QDECREF(input);
QDECREF(args);
+ QDECREF(args_list);
}

/**

monitor.h +1 -0

index b72ea07..499fc1f 100644
@@ -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 {

Quoted 6 lines · Anthony Liguori

Quoted 4 lines · 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.

Quoted 9 lines

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' ] } }

Quoted 176 lines

Quoted 175 lines

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<[email protected]>

---
monitor.c | 72
++++++++++++++++++++++++++++++++++++++++++++++++++----------
monitor.h | 1 +

2 files changed, 61 insertions(+), 12 deletions(-)

monitor.c +9 -0

index a7df995..98e6017 100644
@@ -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;

monitor.c +6 -0

@@ -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)
{

monitor.c +2 -1

@@ -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;

monitor.c +15 -0

@@ -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;

monitor.c +27 -11

@@ -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;

monitor.c +0 -0

@@ -4415,6 +4462,7 @@ err_out:

out:
QDECREF(input);
QDECREF(args);
+ QDECREF(args_list);
}

/**

monitor.h +1 -0

index b72ea07..499fc1f 100644
@@ -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 {

Quoted 5 lines

[1] JSON schema definition:

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

Quoted 2 lines

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

This will end up looking like:

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

Quoted 5 lines · Anthony Liguori

Quoted 5 lines

[1] JSON schema definition:

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

Quoted 8 lines

Quoted 2 lines

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

Quoted 34 lines · Jeff Cody

Quoted 7 lines · Anthony Liguori

Quoted 5 lines · 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.

Quoted 9 lines

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.

Quoted 7 lines

[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

Quoted 178 lines

Quoted 177 lines

Quoted 176 lines

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<[email protected]>

---
monitor.c | 72
++++++++++++++++++++++++++++++++++++++++++++++++++----------
monitor.h | 1 +

2 files changed, 61 insertions(+), 12 deletions(-)

monitor.c +9 -0

index a7df995..98e6017 100644
@@ -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;

monitor.c +6 -0

@@ -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)
{

monitor.c +2 -1

@@ -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;

monitor.c +14 -0

@@ -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;

monitor.c +27 -11

@@ -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;

monitor.c +0 -0

@@ -4415,6 +4462,7 @@ err_out:

out:
QDECREF(input);
QDECREF(args);
+ QDECREF(args_list);
}

/**

monitor.h +1 -0

index b72ea07..499fc1f 100644
@@ -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 {

Quoted 21 lines · Eric Blake

Quoted 5 lines · Anthony Liguori

Quoted 5 lines

[1] JSON schema definition:

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

Quoted 8 lines

Quoted 2 lines

{ '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' ] } }

Quoted 5 lines

and used like:

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

Yes, devlist is a better variable name.

Quoted 4 lines

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

Quoted 29 lines · Jeff Cody

Quoted 6 lines · Anthony Liguori

Quoted 4 lines · 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.

Quoted 9 lines

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.

Quoted 7 lines

[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?

Quoted 47 lines · On Wed, 22 Feb 2012 11:35:26 -0600 Anthony Liguori<[email protected]>…

Quoted 29 lines · Jeff Cody

Quoted 6 lines · Anthony Liguori

Quoted 4 lines · 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.

Quoted 9 lines

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.

Quoted 7 lines

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

Quoted 1 line

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,

Quoted 56 lines · Luiz Capitulino

Quoted 41 lines · On Wed, 22 Feb 2012 11:35:26 -0600 Anthony Liguori<[email protected]>…

Quoted 29 lines · Jeff Cody

Quoted 6 lines · Anthony Liguori

Quoted 4 lines · 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.

Quoted 9 lines

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.

Quoted 7 lines

[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:

Quoted 7 lines

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

Quoted 2 lines

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.