[Qemu-devel] [RFC 00/11]: QMP feature negotiation support

[Qemu-devel] [RFC 00/11]: QMP feature negotiation support

[Luiz Capitulino]

 Feature negotiation allows clients to enable QMP capabilities they are
interested in using. This allows QMP to envolve without breaking old clients.

 A capability is a new QMP feature and/or protocol change which is not part of
the core protocol as defined in the QMP spec.

 Feature negotiation is implemented by defining a set of rules and adding
mode-oriented support.

 The set of rules are:

o All QMP capabilities are disabled by default
o All QMP capabilities must be advertised in the capabilities array
o Commands to enable/disable capabilities must be provided

NOTE: Asynchronous messages are now considered a capability.

 Mode-oriented support adds the following to QMP:

o Two modes: handshake and operational
o By default all QMP Monitors start in handshake mode
o In handshake mode only commands to query/enable/disable QMP capabilities are
  allowed (there are few exceptions)
o Clients can switch to the operational mode at any time
o In Operational mode most commands are allowed and QMP capabilities changes
  made in handshake mode take effect

 Also note that each QMP Monitor has its own mode state and set of capabilities,
this means that if QEMU is started with N QMP Monitors protocol setup done in
one of them doesn't affect the others.

 Session example:

"""
{"QMP": {"capabilities": ["async messages"]}}

{ "execute": "query-qmp-mode" }
{"return": {"mode": "handshake"}}

{ "execute": "change", "arguments": { "device": "vnc", "target": "password", "arg": "1234" } }
{"error": {"class": "QMPInvalidModeCommad", "desc": "The issued command is invalid in this mode", "data": {}}}

{ "execute": "async_msg_enable", "arguments": { "name": "STOP" } }
{"return": {}}

{ "execute": "qmp_switch_mode", "arguments": { "mode": "operational" } }
{"return": {}}

{ "execute": "query-qmp-mode" }
{"return": {"mode": "operational"}}

{ "execute": "change", "arguments": { "device": "vnc", "target": "password", "arg": "1234" } }
{"return": {}}
"""

 TODO:

- Update the spec
- Test more and fix some known issues
- Improve the changelog a bit

[Qemu-devel] [PATCH 01/11] QMP: Initial mode-oriented bits

[Luiz Capitulino]

In order to support feature negotiation in QMP, the Monitor
has to be modified to support different modes of operation.

We need two modes:

o Handshake: where features are negotiated. Only commands
which deal with protocol configuration are allowed, async
messages (as any other protocol capability) are disabled
by default

o Operational: regular Monitor operations. All handlers
(except the protocol configuration ones) are allowed and
async messages enabled in 'handshake' mode are delivered

This commit adds the initial infrastructure to implement this
design, basically it does:

1. Adds the QMPMode data type to MonitorControl and sets
QMODE_HANDSHAKE as the default one

2. Grant permission to 'query-version' and 'query-commands'
to run on handshake mode

Note, however, that these changes are not visable yet and
thus QMP's behavior is still the same.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index cadf422..4b7067a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -76,11 +76,16 @@
  *
  */
 
+/* Handler flags */
+#define HANDLER_HANDSHAKE      0x01 /* allowed to run on handshake mode */
+#define HANDLER_HANDSHAKE_ONLY 0x02 /* can ONLY run on handshake mode */
+
 typedef struct mon_cmd_t {
     const char *name;
     const char *args_type;
     const char *params;
     const char *help;
+    unsigned int flags;
     void (*user_print)(Monitor *mon, const QObject *data);
     union {
         void (*info)(Monitor *mon);
@@ -98,8 +103,14 @@ struct mon_fd_t {
     QLIST_ENTRY(mon_fd_t) next;
 };
 
+typedef enum QMPMode {
+    QMODE_OPERATIONAL,
+    QMODE_HANDSHAKE,
+} QMPMode;
+
 typedef struct MonitorControl {
     QObject *id;
+    QMPMode mode;
     int print_enabled;
     JSONMessageParser parser;
 } MonitorControl;
@@ -2364,6 +2375,7 @@ static const mon_cmd_t info_cmds[] = {
         .args_type  = "",
         .params     = "",
         .help       = "show the version of QEMU",
+        .flags      = HANDLER_HANDSHAKE,
         .user_print = do_info_version_print,
         .mhandler.info_new = do_info_version,
     },
@@ -2372,6 +2384,7 @@ static const mon_cmd_t info_cmds[] = {
         .args_type  = "",
         .params     = "",
         .help       = "list QMP available commands",
+        .flags      = HANDLER_HANDSHAKE,
         .user_print = monitor_user_noop,
         .mhandler.info_new = do_info_commands,
     },
@@ -4264,6 +4277,7 @@ void monitor_init(CharDriverState *chr, int flags)
 
     if (monitor_ctrl_mode(mon)) {
         mon->mc = qemu_mallocz(sizeof(MonitorControl));
+        mon->mc->mode = QMODE_HANDSHAKE;
         /* Control mode requires special handlers */
         qemu_chr_add_handlers(chr, monitor_can_read, monitor_control_read,
                               monitor_control_event, mon);
-- 
1.6.6

[Qemu-devel] [PATCH 02/11] QMP: Introduce 'query-qmp-mode' command

[Luiz Capitulino]

Only valid in QMP and allowed to run in both "handshake" and
"operational" modes.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index 4b7067a..c475a38 100644
--- a/monitor.c
+++ b/monitor.c
@@ -586,6 +586,39 @@ static QObject *get_cmd_dict(const char *name)
 }
 
 /**
+ * do_info_qmp_mode(): Show current QMP mode
+ *
+ * Return a QDict with the following key:
+ *
+ * - "mode": either "handshake" or "operational"
+ *
+ * Example:
+ *
+ * { "mode": "handshake" }
+ */
+static void do_info_qmp_mode(Monitor *mon, QObject **ret_data)
+{
+    const char *mode;
+
+    if (!monitor_ctrl_mode(mon)) {
+        return;
+    }
+
+    switch (mon->mc->mode) {
+        case QMODE_HANDSHAKE:
+            mode = "handshake";
+            break;
+        case QMODE_OPERATIONAL:
+            mode = "operational";
+            break;
+        default:
+            abort();
+    }
+
+    *ret_data = qobject_from_jsonf("{ 'mode': %s }", mode);
+}
+
+/**
  * do_info_commands(): List QMP available commands
  *
  * Each command is represented by a QDict, the returned QObject is a QList
@@ -2619,6 +2652,15 @@ static const mon_cmd_t info_cmds[] = {
         .mhandler.info_new = do_info_migrate,
     },
     {
+        .name       = "qmp-mode",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show current mode",
+        .flags      = HANDLER_HANDSHAKE,
+        .user_print = monitor_user_noop,
+        .mhandler.info_new = do_info_qmp_mode,
+    },
+    {
         .name       = "balloon",
         .args_type  = "",
         .params     = "",
-- 
1.6.6

[Qemu-devel] [PATCH 03/11] QError: Add QMP mode-oriented errors

[Luiz Capitulino]

Two new errors:

- QERR_QMP_INVALID_MODE_NAME
- QERR_QMP_INVALID_MODE_TRANSACTION

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qerror.c |    8 ++++++++
 qerror.h |    6 ++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 5f8fc5d..9dd729a 100644
--- a/qerror.c
+++ b/qerror.c
@@ -105,6 +105,14 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Bad QMP input object",
     },
     {
+        .error_fmt = QERR_QMP_INVALID_MODE_NAME,
+        .desc      = "Mode name %(name) is invalid",
+    },
+    {
+        .error_fmt = QERR_QMP_INVALID_MODE_TRANSACTION,
+        .desc      = "Invalid mode transaction",
+    },
+    {
         .error_fmt = QERR_SET_PASSWD_FAILED,
         .desc      = "Could not set password",
     },
diff --git a/qerror.h b/qerror.h
index 9e220d6..9982d57 100644
--- a/qerror.h
+++ b/qerror.h
@@ -88,6 +88,12 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_QMP_BAD_INPUT_OBJECT \
     "{ 'class': 'QMPBadInputObject', 'data': { 'expected': %s } }"
 
+#define QERR_QMP_INVALID_MODE_NAME \
+    "{ 'class': 'QMPInvalidModeName', 'data': { 'name': %s } }"
+
+#define QERR_QMP_INVALID_MODE_TRANSACTION \
+    "{ 'class': 'QMPInvalidModeTransaction', 'data': {} }"
+
 #define QERR_SET_PASSWD_FAILED \
     "{ 'class': 'SetPasswdFailed', 'data': {} }"
 
-- 
1.6.6

[Qemu-devel] [PATCH 04/11] QMP: Introduce qmp_switch_mode command

[Luiz Capitulino]

It will be used to switch between "handshake" and "operational"
modes. Currently it doesn't have any practical effect, as
mode-oriented support is not enforced yet.

Usage example:

{ "execute": "qmp_switch_mode", "arguments": { "mode": "operational" } }

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c       |   21 +++++++++++++++++++++
 qemu-monitor.hx |   15 +++++++++++++++
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index c475a38..fc6a1ed 100644
--- a/monitor.c
+++ b/monitor.c
@@ -618,6 +618,27 @@ static void do_info_qmp_mode(Monitor *mon, QObject **ret_data)
     *ret_data = qobject_from_jsonf("{ 'mode': %s }", mode);
 }
 
+static void do_qmp_switch_mode(Monitor *mon, const QDict *qdict,
+                               QObject **ret_data)
+{
+    const char *mode;
+
+    if (!monitor_ctrl_mode(mon)) {
+        return;
+    }
+
+    mode = qdict_get_str(qdict, "mode");
+
+    if (mon->mc->mode != QMODE_HANDSHAKE || !strcmp(mode, "handshake")) {
+        /* only handshake -> operational is allowed */
+        qemu_error_new(QERR_QMP_INVALID_MODE_TRANSACTION);
+    } else if (!strcmp(mode, "operational")) {
+        mon->mc->mode = QMODE_OPERATIONAL;
+    } else {
+        qemu_error_new(QERR_QMP_INVALID_MODE_NAME, mode);
+    }
+}
+
 /**
  * do_info_commands(): List QMP available commands
  *
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 1aa7818..eebea09 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -1062,6 +1062,21 @@ STEXI
 Set the encrypted device @var{device} password to @var{password}
 ETEXI
 
+    {
+        .name       = "qmp_switch_mode",
+        .args_type  = "mode:s",
+        .params     = "qmp_switch_mode name",
+        .help       = "switch QMP mode",
+        .flags      = HANDLER_HANDSHAKE_ONLY,
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_qmp_switch_mode,
+    },
+
+STEXI
+@item qmp_switch_mode @var{mode}
+Switch QMP to @var{mode}
+ETEXI
+
 STEXI
 @end table
 ETEXI
-- 
1.6.6

[Qemu-devel] [PATCH 05/11] QMP: advertise asynchronous messages

[Luiz Capitulino]

With feature negotiation support asynchronous messages are
going to behave like any protocol capability, that is, it
is disabled by default, it can be negotiated and has to be
advertised.

TODO: update spec.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/monitor.c b/monitor.c
index fc6a1ed..70a59c7 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4251,6 +4251,21 @@ void monitor_resume(Monitor *mon)
         readline_show_prompt(mon->rs);
 }
 
+/* XXX: Do we need anything fancier here? */
+static QObject *qmp_capabilities(void)
+{
+    int i;
+    QList *qmp_caps;
+    const char *capabilities[] = { "async messages", NULL };
+
+    qmp_caps = qlist_new();
+    for (i = 0; capabilities[i]; i++) {
+        qlist_append(qmp_caps, qstring_from_str(capabilities[i]));
+    }
+
+    return QOBJECT(qmp_caps);
+}
+
 /**
  * monitor_control_event(): Print QMP gretting
  */
@@ -4262,7 +4277,8 @@ static void monitor_control_event(void *opaque, int event)
 
         json_message_parser_init(&mon->mc->parser, handle_qmp_command);
 
-        data = qobject_from_jsonf("{ 'QMP': { 'capabilities': [] } }");
+        data = qobject_from_jsonf("{ 'QMP': { 'capabilities': %p } }",
+                                  qmp_capabilities()); 
         assert(data != NULL);
 
         monitor_json_emitter(mon, data);
-- 
1.6.6

[Qemu-devel] [PATCH 06/11] QMP: Array-based async messages

[Luiz Capitulino]

This commit moves the asynchronous messages names to an array
of structs, so that it can be indexed and searched.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |   67 +++++++++++++++++++++++++++++++------------------------------
 1 files changed, 34 insertions(+), 33 deletions(-)

diff --git a/monitor.c b/monitor.c
index 70a59c7..61c0273 100644
--- a/monitor.c
+++ b/monitor.c
@@ -336,6 +336,38 @@ static void timestamp_put(QDict *qdict)
     qdict_put_obj(qdict, "timestamp", obj);
 }
 
+typedef struct MonitorEventName {
+    const char *name;
+} MonitorEventName;
+
+static const MonitorEventName monitor_events_names[] = {
+    [QEVENT_DEBUG] = {
+        .name  = "DEBUG",
+    },
+    [QEVENT_SHUTDOWN] = {
+        .name  = "SHUTDOWN",
+    },
+    [QEVENT_RESET] = {
+        .name  = "RESET",
+    },
+    [QEVENT_POWERDOWN] = {
+        .name  = "POWERDOWN",
+    },
+    [QEVENT_STOP] = {
+        .name  = "STOP",
+    },
+    [QEVENT_VNC_CONNECTED] = {
+        .name  = "VNC_CONNECTED",
+    },
+    [QEVENT_VNC_INITIALIZED] = {
+        .name  = "VNC_INITIALIZED",
+    },
+    [QEVENT_VNC_DISCONNECTED] = {
+        .name  = "VNC_DISCONNECTED",
+    },
+    { },
+};
+
 /**
  * monitor_protocol_event(): Generate a Monitor event
  *
@@ -344,44 +376,13 @@ static void timestamp_put(QDict *qdict)
 void monitor_protocol_event(MonitorEvent event, QObject *data)
 {
     QDict *qmp;
-    const char *event_name;
     Monitor *mon;
 
-    assert(event < QEVENT_MAX);
-
-    switch (event) {
-        case QEVENT_DEBUG:
-            event_name = "DEBUG";
-            break;
-        case QEVENT_SHUTDOWN:
-            event_name = "SHUTDOWN";
-            break;
-        case QEVENT_RESET:
-            event_name = "RESET";
-            break;
-        case QEVENT_POWERDOWN:
-            event_name = "POWERDOWN";
-            break;
-        case QEVENT_STOP:
-            event_name = "STOP";
-            break;
-        case QEVENT_VNC_CONNECTED:
-            event_name = "VNC_CONNECTED";
-            break;
-        case QEVENT_VNC_INITIALIZED:
-            event_name = "VNC_INITIALIZED";
-            break;
-        case QEVENT_VNC_DISCONNECTED:
-            event_name = "VNC_DISCONNECTED";
-            break;
-        default:
-            abort();
-            break;
-    }
+    assert(event >= 0 && event < QEVENT_MAX);
 
     qmp = qdict_new();
     timestamp_put(qmp);
-    qdict_put(qmp, "event", qstring_from_str(event_name));
+    qdict_put(qmp, "event", qstring_from_str(monitor_events_names[event].name));
     if (data) {
         qobject_incref(data);
         qdict_put_obj(qmp, "data", data);
-- 
1.6.6

[Qemu-devel] [PATCH 07/11] QError: New QERR_ASYNC_MSG_NOT_FOUND

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 9dd729a..12dcc8f 100644
--- a/qerror.c
+++ b/qerror.c
@@ -41,6 +41,10 @@ static const QType qerror_type = {
  */
 static const QErrorStringTable qerror_table[] = {
     {
+        .error_fmt = QERR_ASYNC_MSG_NOT_FOUND,
+        .desc      = "Asynchronous message %(name) has not been found",
+    },
+    {
         .error_fmt = QERR_COMMAND_NOT_FOUND,
         .desc      = "The command %(name) has not been found",
     },
diff --git a/qerror.h b/qerror.h
index 9982d57..8e16b35 100644
--- a/qerror.h
+++ b/qerror.h
@@ -40,6 +40,9 @@ QError *qobject_to_qerror(const QObject *obj);
 /*
  * QError class list
  */
+#define QERR_ASYNC_MSG_NOT_FOUND \
+    "{ 'class': 'AsyncMsgNotFound', 'data': { 'name': %s } }"
+
 #define QERR_COMMAND_NOT_FOUND \
     "{ 'class': 'CommandNotFound', 'data': { 'name': %s } }"
 
-- 
1.6.6

[Qemu-devel] [PATCH 08/11] QMP: Asynchronous messages enable/disable support

[Luiz Capitulino]

This commit disables asynchronous messages by default and
introduces two new QMP commands: async_msg_enable and
async_msg_disable.

Each QMP Monitor has its own set of asynchronous messages,
so for example, if QEMU is run with two QMP Monitors async
messages setup in one of them doesn't affect the other.

To implement this design a bitmap is introduced to the
Monitor struct, each async message is represented by one bit.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c       |   49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 qemu-monitor.hx |   30 ++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 1 deletions(-)

diff --git a/monitor.c b/monitor.c
index 61c0273..321bc3a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -108,11 +108,14 @@ typedef enum QMPMode {
     QMODE_HANDSHAKE,
 } QMPMode;
 
+#define EVENTS_BITMAP_SIZE (QEVENT_MAX / 8)
+
 typedef struct MonitorControl {
     QObject *id;
     QMPMode mode;
     int print_enabled;
     JSONMessageParser parser;
+    uint8_t events_bitmap[EVENTS_BITMAP_SIZE];
 } MonitorControl;
 
 struct Monitor {
@@ -318,6 +321,23 @@ static void monitor_protocol_emitter(Monitor *mon, QObject *data)
     QDECREF(qmp);
 }
 
+static void qevent_set(const Monitor *mon, int64_t event)
+{
+    assert(event >= 0 && event < QEVENT_MAX);
+    mon->mc->events_bitmap[event / 8] |= (1 << (event % 8));
+}
+
+static void qevent_unset(const Monitor *mon, int64_t event)
+{
+    assert(event >= 0 && event < QEVENT_MAX);
+    mon->mc->events_bitmap[event / 8] &= ~(1 << (event % 8));
+}
+
+static int qevent_enabled(const Monitor *mon, int64_t event)
+{
+    return (mon->mc->events_bitmap[event / 8] & (1 << (event % 8)));
+}
+
 static void timestamp_put(QDict *qdict)
 {
     int err;
@@ -389,7 +409,8 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
     }
 
     QLIST_FOREACH(mon, &mon_list, entry) {
-        if (monitor_ctrl_mode(mon)) {
+        if (monitor_ctrl_mode(mon) &&
+            qevent_enabled(mon, event)) {
             monitor_json_emitter(mon, QOBJECT(qmp));
         }
     }
@@ -465,6 +486,32 @@ static void do_commit(Monitor *mon, const QDict *qdict)
     }
 }
 
+static void qevent_set_value(Monitor *mon, const QDict *qdict, int value)
+{
+    int i;
+    const char *name = qdict_get_str(qdict, "name");
+
+    for (i = 0; monitor_events_names[i].name != NULL; i++) {
+        if (!strcmp(monitor_events_names[i].name, name)) {
+            return (value ? qevent_set(mon, i) : qevent_unset(mon, i));
+        }
+    }
+
+    qemu_error_new(QERR_ASYNC_MSG_NOT_FOUND, name);
+}
+
+static void do_async_msg_enable(Monitor *mon, const QDict *qdict,
+                                QObject **ret_data)
+{
+    qevent_set_value(mon, qdict, 1);
+}
+
+static void do_async_msg_disable(Monitor *mon, const QDict *qdict,
+                                 QObject **ret_data)
+{
+    qevent_set_value(mon, qdict, 0);
+}
+
 static void do_info(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     const mon_cmd_t *cmd;
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index eebea09..0f3dfde 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -1077,6 +1077,36 @@ STEXI
 Switch QMP to @var{mode}
 ETEXI
 
+    {
+        .name       = "async_msg_enable",
+        .args_type  = "name:s",
+        .params     = "async_msg_enable name",
+        .help       = "enable an asynchronous message",
+        .flags      = HANDLER_HANDSHAKE_ONLY,
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_async_msg_enable,
+    },
+
+STEXI
+@item async_msg_enable @var{name}
+Enable the asynchronous message @var{name}
+ETEXI
+
+    {
+        .name       = "async_msg_disable",
+        .args_type  = "name:s",
+        .params     = "async_msg_disable name",
+        .help       = "disable an asynchronous message",
+        .flags      = HANDLER_HANDSHAKE_ONLY,
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_async_msg_disable,
+    },
+
+STEXI
+@item async_msg_disable @var{name}
+Disable the asynchronous message @var{name}
+ETEXI
+
 STEXI
 @end table
 ETEXI
-- 
1.6.6

[Qemu-devel] [PATCH 09/11] Monitor: Introduce find_info_cmd()

[Luiz Capitulino]

Move the code used to locate an info command entry to its
own function, as it's going to be used by other functions.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/monitor.c b/monitor.c
index 321bc3a..34df1cc 100644
--- a/monitor.c
+++ b/monitor.c
@@ -512,6 +512,19 @@ static void do_async_msg_disable(Monitor *mon, const QDict *qdict,
     qevent_set_value(mon, qdict, 0);
 }
 
+static const mon_cmd_t *monitor_find_info_command(const char *name)
+{
+    const mon_cmd_t *cmd;
+
+    for (cmd = info_cmds; cmd->name != NULL; cmd++) {
+        if (compare_cmd(name, cmd->name)) {
+            return cmd;
+        }
+    }
+
+    return NULL;
+}
+
 static void do_info(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     const mon_cmd_t *cmd;
@@ -522,12 +535,8 @@ static void do_info(Monitor *mon, const QDict *qdict, QObject **ret_data)
         goto help;
     }
 
-    for (cmd = info_cmds; cmd->name != NULL; cmd++) {
-        if (compare_cmd(item, cmd->name))
-            break;
-    }
-
-    if (cmd->name == NULL) {
+    cmd = monitor_find_info_command(item);
+    if (!cmd) {
         if (monitor_ctrl_mode(mon)) {
             qemu_error_new(QERR_COMMAND_NOT_FOUND, item);
             return;
-- 
1.6.6

[Qemu-devel] [PATCH 10/11] QError: New QERR_QMP_INVALID_MODE_COMMAND

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 12dcc8f..52cccc6 100644
--- a/qerror.c
+++ b/qerror.c
@@ -109,6 +109,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Bad QMP input object",
     },
     {
+        .error_fmt = QERR_QMP_INVALID_MODE_COMMAND,
+        .desc      = "The issued command is invalid in this mode",
+    },
+    {
         .error_fmt = QERR_QMP_INVALID_MODE_NAME,
         .desc      = "Mode name %(name) is invalid",
     },
diff --git a/qerror.h b/qerror.h
index 8e16b35..c0deacc 100644
--- a/qerror.h
+++ b/qerror.h
@@ -91,6 +91,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_QMP_BAD_INPUT_OBJECT \
     "{ 'class': 'QMPBadInputObject', 'data': { 'expected': %s } }"
 
+#define QERR_QMP_INVALID_MODE_COMMAND \
+    "{ 'class': 'QMPInvalidModeCommad', 'data': {} }"
+
 #define QERR_QMP_INVALID_MODE_NAME \
     "{ 'class': 'QMPInvalidModeName', 'data': { 'name': %s } }"
 
-- 
1.6.6

[Qemu-devel] [PATCH 11/11] QMP: Enable feature negotiation support

[Luiz Capitulino]

This is done by enforcing the following mode-oriented rules:

- QMP is started in handshake mode
- In handshake mode all protocol capabilities are disabled
  and (apart of a few exceptions) only commands which
  query/enable/disable them are allowed
- Asynchronous messages are now considered a capability
- Clients can change to the operational mode (where capabilities'
  changes take effect and most commands are allowed) at any
  time
- Each QMP Monitor has its own set of capabilities, changes
  made to one of them don't affect the others

Also note that all these changes should have no effect in
the user Monitor.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |   43 ++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 42 insertions(+), 1 deletions(-)

diff --git a/monitor.c b/monitor.c
index 34df1cc..0ca0941 100644
--- a/monitor.c
+++ b/monitor.c
@@ -410,7 +410,8 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
 
     QLIST_FOREACH(mon, &mon_list, entry) {
         if (monitor_ctrl_mode(mon) &&
-            qevent_enabled(mon, event)) {
+            qevent_enabled(mon, event) &&
+            mon->mc->mode == QMODE_OPERATIONAL) {
             monitor_json_emitter(mon, QOBJECT(qmp));
         }
     }
@@ -4166,12 +4167,38 @@ static int monitor_check_qmp_args(const mon_cmd_t *cmd, QDict *args)
     return err;
 }
 
+static int qmp_mode_invalid(const Monitor *mon, unsigned int cmd_flags)
+{
+    switch (mon->mc->mode) {
+        case QMODE_OPERATIONAL:
+            if (cmd_flags & HANDLER_HANDSHAKE_ONLY) {
+                goto mode_error;
+            }
+            break;
+        case QMODE_HANDSHAKE:
+            if (!((cmd_flags & HANDLER_HANDSHAKE) ||
+                (cmd_flags & HANDLER_HANDSHAKE_ONLY))) {
+                goto mode_error;
+            }
+            break;
+        default:
+            abort();
+    }
+
+    return 0;
+
+mode_error:
+    qemu_error_new(QERR_QMP_INVALID_MODE_COMMAND);
+    return 1;
+}
+
 static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
 {
     int err;
     QObject *obj;
     QDict *input, *args;
     const mon_cmd_t *cmd;
+    unsigned int cmd_flags;
     Monitor *mon = cur_mon;
     const char *cmd_name, *info_item;
 
@@ -4213,6 +4240,15 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
         qemu_error_new(QERR_COMMAND_NOT_FOUND, cmd_name);
         goto err_input;
     } else if (strstart(cmd_name, "query-", &info_item)) {
+        /* check it exists and get its flags */
+        cmd = monitor_find_info_command(info_item);
+        if (!cmd) {
+            qemu_error_new(QERR_COMMAND_NOT_FOUND, cmd_name);
+            goto err_input;
+        }
+        cmd_flags = cmd->flags;
+
+        /* setup 'info' to call it */
         cmd = monitor_find_command("info");
         qdict_put_obj(input, "arguments",
                       qobject_from_jsonf("{ 'item': %s }", info_item));
@@ -4222,6 +4258,11 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
             qemu_error_new(QERR_COMMAND_NOT_FOUND, cmd_name);
             goto err_input;
         }
+        cmd_flags = cmd->flags;
+    }
+
+    if (qmp_mode_invalid(mon, cmd_flags)) {
+        goto err_input;
     }
 
     obj = qdict_get(input, "arguments");
-- 
1.6.6

Re: [Qemu-devel] [RFC 00/11]: QMP feature negotiation support

[Markus Armbruster]

A few quick questions before I dive into the patches...

Luiz Capitulino <lcapitulino@redhat.com> writes:

>  Feature negotiation allows clients to enable QMP capabilities they are
> interested in using. This allows QMP to envolve without breaking old clients.
>
>  A capability is a new QMP feature and/or protocol change which is not part of
> the core protocol as defined in the QMP spec.
>
>  Feature negotiation is implemented by defining a set of rules and adding
> mode-oriented support.
>
>  The set of rules are:
>
> o All QMP capabilities are disabled by default
> o All QMP capabilities must be advertised in the capabilities array
> o Commands to enable/disable capabilities must be provided
>
> NOTE: Asynchronous messages are now considered a capability.
>
>  Mode-oriented support adds the following to QMP:
>
> o Two modes: handshake and operational
> o By default all QMP Monitors start in handshake mode

"By default"?  Is there a way to start a QMP monitor in another mode?

> o In handshake mode only commands to query/enable/disable QMP capabilities are
>   allowed (there are few exceptions)

Note to self: check out the exception, and why we might want them.

> o Clients can switch to the operational mode at any time

Can they switch back?  I hope not.

> o In Operational mode most commands are allowed and QMP capabilities changes
>   made in handshake mode take effect
>
>  Also note that each QMP Monitor has its own mode state and set of capabilities,
> this means that if QEMU is started with N QMP Monitors protocol setup done in
> one of them doesn't affect the others.
>
>  Session example:
>
> """
> {"QMP": {"capabilities": ["async messages"]}}
>
> { "execute": "query-qmp-mode" }
> {"return": {"mode": "handshake"}}

Why would clients want to query the mode?

> { "execute": "change", "arguments": { "device": "vnc", "target": "password", "arg": "1234" } }
> {"error": {"class": "QMPInvalidModeCommad", "desc": "The issued command is invalid in this mode", "data": {}}}

I'd treat this like a completely unknown command.

> { "execute": "async_msg_enable", "arguments": { "name": "STOP" } }
> {"return": {}}
>
> { "execute": "qmp_switch_mode", "arguments": { "mode": "operational" } }
> {"return": {}}

Do we envisage mode transitions other than handshake -> operational?

> { "execute": "query-qmp-mode" }
> {"return": {"mode": "operational"}}
>
> { "execute": "change", "arguments": { "device": "vnc", "target": "password", "arg": "1234" } }
> {"return": {}}
> """
>
>  TODO:
>
> - Update the spec
> - Test more and fix some known issues
> - Improve the changelog a bit

Re: [Qemu-devel] [RFC 00/11]: QMP feature negotiation support

[Luiz Capitulino]

On Fri, 22 Jan 2010 11:21:22 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> A few quick questions before I dive into the patches...
> 
> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> >  Feature negotiation allows clients to enable QMP capabilities they are
> > interested in using. This allows QMP to envolve without breaking old clients.
> >
> >  A capability is a new QMP feature and/or protocol change which is not part of
> > the core protocol as defined in the QMP spec.
> >
> >  Feature negotiation is implemented by defining a set of rules and adding
> > mode-oriented support.
> >
> >  The set of rules are:
> >
> > o All QMP capabilities are disabled by default
> > o All QMP capabilities must be advertised in the capabilities array
> > o Commands to enable/disable capabilities must be provided
> >
> > NOTE: Asynchronous messages are now considered a capability.
> >
> >  Mode-oriented support adds the following to QMP:
> >
> > o Two modes: handshake and operational
> > o By default all QMP Monitors start in handshake mode
> 
> "By default"?  Is there a way to start a QMP monitor in another mode?

 No, you think it's worth or is it just about the English?

> > o In handshake mode only commands to query/enable/disable QMP capabilities are
> >   allowed (there are few exceptions)
> 
> Note to self: check out the exception, and why we might want them.

 The following handlers are handshake-only:

- qmp_switch_mode
- async_msg_enable
- async_msg_disable

 The following handlers are allowed to run on both modes:

- query-version
- query-qmp-mode
- query-commands

 Also, all the self-description commands (query-async-msg,
query-errors etc) would be allowed on both modes.

 So, the only handler which is not completely related to feature
negotiation is query-version. This is only a guess, but I think
it might be worth to let clients learn the QEMU version they are
talking to before setting protocol features.

> > o Clients can switch to the operational mode at any time
> 
> Can they switch back?  I hope not.

 No, they can't. The only transition allowed is handshake -> operational.

> > o In Operational mode most commands are allowed and QMP capabilities changes
> >   made in handshake mode take effect
> >
> >  Also note that each QMP Monitor has its own mode state and set of capabilities,
> > this means that if QEMU is started with N QMP Monitors protocol setup done in
> > one of them doesn't affect the others.
> >
> >  Session example:
> >
> > """
> > {"QMP": {"capabilities": ["async messages"]}}
> >
> > { "execute": "query-qmp-mode" }
> > {"return": {"mode": "handshake"}}
> 
> Why would clients want to query the mode?

 It's more useful for testing purposes.

> > { "execute": "change", "arguments": { "device": "vnc", "target": "password", "arg": "1234" } }
> > {"error": {"class": "QMPInvalidModeCommad", "desc": "The issued command is invalid in this mode", "data": {}}}
> 
> I'd treat this like a completely unknown command.

 Really? This would simplify things a bit.

> > { "execute": "async_msg_enable", "arguments": { "name": "STOP" } }
> > {"return": {}}
> >
> > { "execute": "qmp_switch_mode", "arguments": { "mode": "operational" } }
> > {"return": {}}
> 
> Do we envisage mode transitions other than handshake -> operational?

 Today we don't, but this is about forward compatibility support right? :)

 So, IMO it makes sense to have a more general command instead of
qmp_switch_operational.

Re: [Qemu-devel] [RFC 00/11]: QMP feature negotiation support

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 22 Jan 2010 11:21:22 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> A few quick questions before I dive into the patches...
>> 
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> >  Feature negotiation allows clients to enable QMP capabilities they are
>> > interested in using. This allows QMP to envolve without breaking old clients.
>> >
>> >  A capability is a new QMP feature and/or protocol change which is not part of
>> > the core protocol as defined in the QMP spec.
>> >
>> >  Feature negotiation is implemented by defining a set of rules and adding
>> > mode-oriented support.
>> >
>> >  The set of rules are:
>> >
>> > o All QMP capabilities are disabled by default
>> > o All QMP capabilities must be advertised in the capabilities array
>> > o Commands to enable/disable capabilities must be provided
>> >
>> > NOTE: Asynchronous messages are now considered a capability.
>> >
>> >  Mode-oriented support adds the following to QMP:
>> >
>> > o Two modes: handshake and operational
>> > o By default all QMP Monitors start in handshake mode
>> 
>> "By default"?  Is there a way to start a QMP monitor in another mode?
>
>  No, you think it's worth or is it just about the English?

Just about the language.

>> > o In handshake mode only commands to query/enable/disable QMP capabilities are
>> >   allowed (there are few exceptions)
>> 
>> Note to self: check out the exception, and why we might want them.
>
>  The following handlers are handshake-only:
>
> - qmp_switch_mode
> - async_msg_enable
> - async_msg_disable

Two commands per feature?  I'd rather have a single "feature NAME
VALUE", where NAME identifies the feature, and VALUE specifies whether
to turn it on or off.

>  The following handlers are allowed to run on both modes:
>
> - query-version
> - query-qmp-mode
> - query-commands
>
>  Also, all the self-description commands (query-async-msg,
> query-errors etc) would be allowed on both modes.
>
>  So, the only handler which is not completely related to feature
> negotiation is query-version. This is only a guess, but I think
> it might be worth to let clients learn the QEMU version they are
> talking to before setting protocol features.

I'd simply announce it in the greeting, just like the features.

But I don't mind supporting selected query- commands in handshake mode,
if that's easy to do.

>> > o Clients can switch to the operational mode at any time
>> 
>> Can they switch back?  I hope not.
>
>  No, they can't. The only transition allowed is handshake -> operational.
>
>> > o In Operational mode most commands are allowed and QMP capabilities changes
>> >   made in handshake mode take effect
>> >
>> >  Also note that each QMP Monitor has its own mode state and set of capabilities,
>> > this means that if QEMU is started with N QMP Monitors protocol setup done in
>> > one of them doesn't affect the others.
>> >
>> >  Session example:
>> >
>> > """
>> > {"QMP": {"capabilities": ["async messages"]}}
>> >
>> > { "execute": "query-qmp-mode" }
>> > {"return": {"mode": "handshake"}}
>> 
>> Why would clients want to query the mode?
>
>  It's more useful for testing purposes.

Just try a command that works only in one of the modes ;)

>> > { "execute": "change", "arguments": { "device": "vnc", "target": "password", "arg": "1234" } }
>> > {"error": {"class": "QMPInvalidModeCommad", "desc": "The issued command is invalid in this mode", "data": {}}}
>> 
>> I'd treat this like a completely unknown command.
>
>  Really? This would simplify things a bit.

Simple is good.

>> > { "execute": "async_msg_enable", "arguments": { "name": "STOP" } }
>> > {"return": {}}
>> >
>> > { "execute": "qmp_switch_mode", "arguments": { "mode": "operational" } }
>> > {"return": {}}
>> 
>> Do we envisage mode transitions other than handshake -> operational?
>
>  Today we don't, but this is about forward compatibility support right? :)
>
>  So, IMO it makes sense to have a more general command instead of
> qmp_switch_operational.

I'd say transitioning from initial feature negotiation to normal
operations is a special case, so generality is not needed here.  I'm
fine with you providing it anyway.

Re: [Qemu-devel] [RFC 00/11]: QMP feature negotiation support

[Anthony Liguori]

On 01/21/2010 03:09 PM, Luiz Capitulino wrote:
> """
> {"QMP": {"capabilities": ["async messages"]}}
>
> { "execute": "query-qmp-mode" }
> {"return": {"mode": "handshake"}}
>
> { "execute": "change", "arguments": { "device": "vnc", "target": "password", "arg": "1234" } }
> {"error": {"class": "QMPInvalidModeCommad", "desc": "The issued command is invalid in this mode", "data": {}}}
>
> { "execute": "async_msg_enable", "arguments": { "name": "STOP" } }
> {"return": {}}
>    

Maybe:

enable-capability "async messages"
disable-capability "async messages"

I think that's a bit more obvious and it means that a client doesn't 
have to maintain a mapping of features -> enable functions.  It's also 
strange to use an enable command to disable something.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 08/11] QMP: Asynchronous messages enable/disable support

[Anthony Liguori]

On 01/21/2010 03:09 PM, Luiz Capitulino wrote:
> This commit disables asynchronous messages by default and
> introduces two new QMP commands: async_msg_enable and
> async_msg_disable.
>
> Each QMP Monitor has its own set of asynchronous messages,
> so for example, if QEMU is run with two QMP Monitors async
> messages setup in one of them doesn't affect the other.
>
> To implement this design a bitmap is introduced to the
> Monitor struct, each async message is represented by one bit.
>
> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>    

Ah, I see I was a little confused.

I'd suggest making async message masking an independent mechanism.  
Capabilities should strictly deal with protocol changes, not feature 
details.

For instance, protocol timestamps are something would reasonable 
considered a capability.  Extended data types would be a capability.

Another way to look at it, is that if you're writing a client library, 
the capability negotiation should be entirely invisible to the end API user.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 08/11] QMP: Asynchronous messages enable/disable support

[Luiz Capitulino]

On Fri, 22 Jan 2010 12:05:19 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 01/21/2010 03:09 PM, Luiz Capitulino wrote:
> > This commit disables asynchronous messages by default and
> > introduces two new QMP commands: async_msg_enable and
> > async_msg_disable.
> >
> > Each QMP Monitor has its own set of asynchronous messages,
> > so for example, if QEMU is run with two QMP Monitors async
> > messages setup in one of them doesn't affect the other.
> >
> > To implement this design a bitmap is introduced to the
> > Monitor struct, each async message is represented by one bit.
> >
> > Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> >    
> 
> Ah, I see I was a little confused.
> 
> I'd suggest making async message masking an independent mechanism.  
> Capabilities should strictly deal with protocol changes, not feature 
> details.

 To summarize (after a IRC talk): async messages are considered a
capability and should be enabled during the negotiation phase but
the masking of particular messages are not and can be done at
any time after the negotiation phase.

 I'm ok with that, Markus?

Re: [Qemu-devel] [PATCH 08/11] QMP: Asynchronous messages enable/disable support

[Anthony Liguori]

On 01/22/2010 02:09 PM, Luiz Capitulino wrote:
> On Fri, 22 Jan 2010 12:05:19 -0600
> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>
>    
>> On 01/21/2010 03:09 PM, Luiz Capitulino wrote:
>>      
>>> This commit disables asynchronous messages by default and
>>> introduces two new QMP commands: async_msg_enable and
>>> async_msg_disable.
>>>
>>> Each QMP Monitor has its own set of asynchronous messages,
>>> so for example, if QEMU is run with two QMP Monitors async
>>> messages setup in one of them doesn't affect the other.
>>>
>>> To implement this design a bitmap is introduced to the
>>> Monitor struct, each async message is represented by one bit.
>>>
>>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>>>
>>>        
>> Ah, I see I was a little confused.
>>
>> I'd suggest making async message masking an independent mechanism.
>> Capabilities should strictly deal with protocol changes, not feature
>> details.
>>      
>   To summarize (after a IRC talk): async messages are considered a
> capability and should be enabled during the negotiation phase but
> the masking of particular messages are not and can be done at
> any time after the negotiation phase.
>    

Just to further clarify, the mental model I have of capabilities is that 
they are things that affect the protocol itself.  Additional features 
(new command options, new commands, new async messages) are things that 
are enabled/discovered in a different way.

Async messages themselves are a capability since it changes the 
protocol.  Timestamps would be another capability and a new data type 
(like an uint64 type) would be a new capability.

>   I'm ok with that, Markus?
>    

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 08/11] QMP: Asynchronous messages enable/disable support

[Avi Kivity]

On 01/22/2010 08:05 PM, Anthony Liguori wrote:
> On 01/21/2010 03:09 PM, Luiz Capitulino wrote:
>> This commit disables asynchronous messages by default and
>> introduces two new QMP commands: async_msg_enable and
>> async_msg_disable.
>>
>> Each QMP Monitor has its own set of asynchronous messages,
>> so for example, if QEMU is run with two QMP Monitors async
>> messages setup in one of them doesn't affect the other.
>>
>> To implement this design a bitmap is introduced to the
>> Monitor struct, each async message is represented by one bit.
>>
>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>
> Ah, I see I was a little confused.
>
> I'd suggest making async message masking an independent mechanism.  
> Capabilities should strictly deal with protocol changes, not feature 
> details.
>
> For instance, protocol timestamps are something would reasonable 
> considered a capability.  Extended data types would be a capability.
>
> Another way to look at it, is that if you're writing a client library, 
> the capability negotiation should be entirely invisible to the end API 
> user.
>

I agree with that, but we can look at async messages as a baseline 
protocol capability (thus no negotiation required), and the new command 
only enabled individual messages.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 08/11] QMP: Asynchronous messages enable/disable support

[Avi Kivity]

On 01/21/2010 11:09 PM, Luiz Capitulino wrote:
> This commit disables asynchronous messages by default and
> introduces two new QMP commands: async_msg_enable and
> async_msg_disable.
>
> Each QMP Monitor has its own set of asynchronous messages,
> so for example, if QEMU is run with two QMP Monitors async
> messages setup in one of them doesn't affect the other.
>
> To implement this design a bitmap is introduced to the
> Monitor struct, each async message is represented by one bit.
>
>    

A bitmap is an overkill here, an array of booleans should suffice.

>
> +#define EVENTS_BITMAP_SIZE (QEVENT_MAX / 8)
>    

Doesn't that underflow if QEVENT_MAX is not a multiple of 8?

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 08/11] QMP: Asynchronous messages enable/disable support

[Jamie Lokier]

Avi Kivity wrote:
> On 01/22/2010 08:05 PM, Anthony Liguori wrote:
> >On 01/21/2010 03:09 PM, Luiz Capitulino wrote:
> >>This commit disables asynchronous messages by default and
> >>introduces two new QMP commands: async_msg_enable and
> >>async_msg_disable.
> >>
> >>Each QMP Monitor has its own set of asynchronous messages,
> >>so for example, if QEMU is run with two QMP Monitors async
> >>messages setup in one of them doesn't affect the other.
> >>
> >>To implement this design a bitmap is introduced to the
> >>Monitor struct, each async message is represented by one bit.
> >>
> >>Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> >
> >Ah, I see I was a little confused.
> >
> >I'd suggest making async message masking an independent mechanism.  
> >Capabilities should strictly deal with protocol changes, not feature 
> >details.
> >
> >For instance, protocol timestamps are something would reasonable 
> >considered a capability.  Extended data types would be a capability.
> >
> >Another way to look at it, is that if you're writing a client library, 
> >the capability negotiation should be entirely invisible to the end API 
> >user.
> 
> I agree with that, but we can look at async messages as a baseline 
> protocol capability (thus no negotiation required), and the new command 
> only enabled individual messages.

I'd like to be able to connect and be sure not to receive any async
messages, from simple scripts with simple output parsing.

If async messages can only be received as a result of commands which
trigger individual messages, that will be achieved.

But it would be a nice little bonus if disabling async messages caused
all slow commands to be synchronous - that is, the async result
message becomes the command's synchronous result.

-- Jamie

Re: [Qemu-devel] [PATCH 08/11] QMP: Asynchronous messages enable/disable support

[Anthony Liguori]

On 01/24/2010 04:34 AM, Avi Kivity wrote:
> On 01/22/2010 08:05 PM, Anthony Liguori wrote:
>> On 01/21/2010 03:09 PM, Luiz Capitulino wrote:
>>> This commit disables asynchronous messages by default and
>>> introduces two new QMP commands: async_msg_enable and
>>> async_msg_disable.
>>>
>>> Each QMP Monitor has its own set of asynchronous messages,
>>> so for example, if QEMU is run with two QMP Monitors async
>>> messages setup in one of them doesn't affect the other.
>>>
>>> To implement this design a bitmap is introduced to the
>>> Monitor struct, each async message is represented by one bit.
>>>
>>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>>
>> Ah, I see I was a little confused.
>>
>> I'd suggest making async message masking an independent mechanism.  
>> Capabilities should strictly deal with protocol changes, not feature 
>> details.
>>
>> For instance, protocol timestamps are something would reasonable 
>> considered a capability.  Extended data types would be a capability.
>>
>> Another way to look at it, is that if you're writing a client 
>> library, the capability negotiation should be entirely invisible to 
>> the end API user.
>>
>
> I agree with that, but we can look at async messages as a baseline 
> protocol capability (thus no negotiation required), and the new 
> command only enabled individual messages.

To be honest, I don't think there's really a need to mask individual 
messages.  A client can always ignore messages it doesn't care about.  
There is no side effect of receiving a message so there is no functional 
implication of receiving messages you don't care about.

The only time it would matter is if we had a really high volume of 
messages.  I'd suggest waiting until a message is introduced that could 
potentially have a high rate and then implement a mechanism to mask it.  
For now, it just adds unnecessary complexity.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 08/11] QMP: Asynchronous messages enable/disable support

[Avi Kivity]

On 01/24/2010 04:04 PM, Anthony Liguori wrote:
>> I agree with that, but we can look at async messages as a baseline 
>> protocol capability (thus no negotiation required), and the new 
>> command only enabled individual messages.
>
>
> To be honest, I don't think there's really a need to mask individual 
> messages.  A client can always ignore messages it doesn't care about.  
> There is no side effect of receiving a message so there is no 
> functional implication of receiving messages you don't care about.
>
> The only time it would matter is if we had a really high volume of 
> messages.  I'd suggest waiting until a message is introduced that 
> could potentially have a high rate and then implement a mechanism to 
> mask it.  For now, it just adds unnecessary complexity.

Fair enough.  But then, why can't all clients do that?  Dropping an 
async notification is maybe one line of code.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 08/11] QMP: Asynchronous messages enable/disable support

[Anthony Liguori]

On 01/24/2010 08:17 AM, Avi Kivity wrote:
> On 01/24/2010 04:04 PM, Anthony Liguori wrote:
>>> I agree with that, but we can look at async messages as a baseline 
>>> protocol capability (thus no negotiation required), and the new 
>>> command only enabled individual messages.
>>
>>
>> To be honest, I don't think there's really a need to mask individual 
>> messages.  A client can always ignore messages it doesn't care 
>> about.  There is no side effect of receiving a message so there is no 
>> functional implication of receiving messages you don't care about.
>>
>> The only time it would matter is if we had a really high volume of 
>> messages.  I'd suggest waiting until a message is introduced that 
>> could potentially have a high rate and then implement a mechanism to 
>> mask it.  For now, it just adds unnecessary complexity.
>
> Fair enough.  But then, why can't all clients do that?  Dropping an 
> async notification is maybe one line of code.

I agree.  The only argument I can imagine for masking is if we had a 
really, really high volume event that caused bandwidth issues.  I don't 
think that's an appropriate use of async messages though so I don't 
think this will ever happen.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 08/11] QMP: Asynchronous messages enable/disable support

[Anthony Liguori]

On 01/24/2010 05:07 AM, Jamie Lokier wrote:
> Avi Kivity wrote:
>    
>> On 01/22/2010 08:05 PM, Anthony Liguori wrote:
>>      
>>> On 01/21/2010 03:09 PM, Luiz Capitulino wrote:
>>>        
>>>> This commit disables asynchronous messages by default and
>>>> introduces two new QMP commands: async_msg_enable and
>>>> async_msg_disable.
>>>>
>>>> Each QMP Monitor has its own set of asynchronous messages,
>>>> so for example, if QEMU is run with two QMP Monitors async
>>>> messages setup in one of them doesn't affect the other.
>>>>
>>>> To implement this design a bitmap is introduced to the
>>>> Monitor struct, each async message is represented by one bit.
>>>>
>>>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>>>>          
>>> Ah, I see I was a little confused.
>>>
>>> I'd suggest making async message masking an independent mechanism.
>>> Capabilities should strictly deal with protocol changes, not feature
>>> details.
>>>
>>> For instance, protocol timestamps are something would reasonable
>>> considered a capability.  Extended data types would be a capability.
>>>
>>> Another way to look at it, is that if you're writing a client library,
>>> the capability negotiation should be entirely invisible to the end API
>>> user.
>>>        
>> I agree with that, but we can look at async messages as a baseline
>> protocol capability (thus no negotiation required), and the new command
>> only enabled individual messages.
>>      
> I'd like to be able to connect and be sure not to receive any async
> messages, from simple scripts with simple output parsing.
>    

You can't have simple output parsing with QMP.  You need a full JSON 
stack.  The simplest script would be a python script that uses the 
builtin json support.  Having async messages means that you'll have to 
loop on recv in order make sure that the response is a command response 
vs. an async message.  It's just a few lines more of code so I have a 
hard time believing it's really a problem.

But what you probably want is a python QMP library and that would mean 
you wouldn't need the few more lines of code.

> If async messages can only be received as a result of commands which
> trigger individual messages, that will be achieved.
>
> But it would be a nice little bonus if disabling async messages caused
> all slow commands to be synchronous - that is, the async result
> message becomes the command's synchronous result.
>    

I know what you're getting at but I think it's the wrong target for 
QMP.  The goal is not to allow simple, hacky clients, but to provide a 
robust management API.  It should not be difficult to use, but parsing 
it in a shell with awk is not a requirement in my mind :-)

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 08/11] QMP: Asynchronous messages enable/disable support

[Jamie Lokier]

Anthony Liguori wrote:
> >I'd like to be able to connect and be sure not to receive any async
> >messages, from simple scripts with simple output parsing.
> 
> You can't have simple output parsing with QMP.  You need a full JSON 
> stack.  The simplest script would be a python script that uses the 
> builtin json support.  Having async messages means that you'll have to 
> loop on recv in order make sure that the response is a command response 
> vs. an async message.  It's just a few lines more of code so I have a 
> hard time believing it's really a problem.
> 
> But what you probably want is a python QMP library and that would mean 
> you wouldn't need the few more lines of code.

You're right.  To be honest, parsing JSON can be done in a single Perl
regexp; a "full JSON stack" isn't much.

On that note, it'd be good if the end of a QMP message is framed with
something that can't appear inside the JSON value, without having to
parse the JSON incrementally on each partial read().  There are plenty
of short character sequences to choose from that can't appear.  Some
JSON parsers expect a whole well-formed expression or throw a parse
error - it's what people do over HTTP after all.  So you wait until
you think you've read a whole one, then pass it to the JSON parser.

> >If async messages can only be received as a result of commands which
> >trigger individual messages, that will be achieved.
> >
> >But it would be a nice little bonus if disabling async messages caused
> >all slow commands to be synchronous - that is, the async result
> >message becomes the command's synchronous result.
> 
> I know what you're getting at but I think it's the wrong target for 
> QMP.  The goal is not to allow simple, hacky clients, but to provide a 
> robust management API.  It should not be difficult to use, but parsing 
> it in a shell with awk is not a requirement in my mind :-)

I agree - not a requirement.  Just seemed nice if that turned out to
be a natural thing to do anyway.

-- Jamie

Re: [Qemu-devel] [PATCH 08/11] QMP: Asynchronous messages enable/disable support

[Luiz Capitulino]

On Sun, 24 Jan 2010 18:35:14 +0000
Jamie Lokier <jamie@shareable.org> wrote:

> Anthony Liguori wrote:
> > >I'd like to be able to connect and be sure not to receive any async
> > >messages, from simple scripts with simple output parsing.
> > 
> > You can't have simple output parsing with QMP.  You need a full JSON 
> > stack.  The simplest script would be a python script that uses the 
> > builtin json support.  Having async messages means that you'll have to 
> > loop on recv in order make sure that the response is a command response 
> > vs. an async message.  It's just a few lines more of code so I have a 
> > hard time believing it's really a problem.
> > 
> > But what you probably want is a python QMP library and that would mean 
> > you wouldn't need the few more lines of code.
> 
> You're right.  To be honest, parsing JSON can be done in a single Perl
> regexp; a "full JSON stack" isn't much.
> 
> On that note, it'd be good if the end of a QMP message is framed with
> something that can't appear inside the JSON value, without having to
> parse the JSON incrementally on each partial read().  There are plenty
> of short character sequences to choose from that can't appear.  Some
> JSON parsers expect a whole well-formed expression or throw a parse
> error - it's what people do over HTTP after all.  So you wait until
> you think you've read a whole one, then pass it to the JSON parser.

 The Monitor sends a CRLF sequence at the end of each line and I have
maintained that behaivor for QMP, but it's hard to _guarantee_ that this
sequence won't appear inside a json-string.

 In practice it doesn't, afaik.

Re: [Qemu-devel] [PATCH 08/11] QMP: Asynchronous messages enable/disable support

[Luiz Capitulino]

On Sun, 24 Jan 2010 08:19:53 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 01/24/2010 08:17 AM, Avi Kivity wrote:
> > On 01/24/2010 04:04 PM, Anthony Liguori wrote:
> >>> I agree with that, but we can look at async messages as a baseline 
> >>> protocol capability (thus no negotiation required), and the new 
> >>> command only enabled individual messages.
> >>
> >>
> >> To be honest, I don't think there's really a need to mask individual 
> >> messages.  A client can always ignore messages it doesn't care 
> >> about.  There is no side effect of receiving a message so there is no 
> >> functional implication of receiving messages you don't care about.
> >>
> >> The only time it would matter is if we had a really high volume of 
> >> messages.  I'd suggest waiting until a message is introduced that 
> >> could potentially have a high rate and then implement a mechanism to 
> >> mask it.  For now, it just adds unnecessary complexity.
> >
> > Fair enough.  But then, why can't all clients do that?  Dropping an 
> > async notification is maybe one line of code.
> 
> I agree.  The only argument I can imagine for masking is if we had a 
> really, really high volume event that caused bandwidth issues.  I don't 
> think that's an appropriate use of async messages though so I don't 
> think this will ever happen.

 The only real client writer we have thought it would be a good
idea to disable them by default.

 If we don't this now and add a masking command later, then clients
that don't want certain messages will have to deal with them between
the window of qmp_swicth_mode and masking.

 I don't have strong opinions here though.

[Qemu-devel] Re: [PATCH 08/11] QMP: Asynchronous messages enable/disable support

[Luiz Capitulino]

On Sun, 24 Jan 2010 12:36:39 +0200
Avi Kivity <avi@redhat.com> wrote:

> On 01/21/2010 11:09 PM, Luiz Capitulino wrote:
> > This commit disables asynchronous messages by default and
> > introduces two new QMP commands: async_msg_enable and
> > async_msg_disable.
> >
> > Each QMP Monitor has its own set of asynchronous messages,
> > so for example, if QEMU is run with two QMP Monitors async
> > messages setup in one of them doesn't affect the other.
> >
> > To implement this design a bitmap is introduced to the
> > Monitor struct, each async message is represented by one bit.
> >
> >    
> 
> A bitmap is an overkill here, an array of booleans should suffice.

 Ok.

> > +#define EVENTS_BITMAP_SIZE (QEVENT_MAX / 8)
> >    
> 
> Doesn't that underflow if QEVENT_MAX is not a multiple of 8?

 Yes.

Re: [Qemu-devel] [PATCH 08/11] QMP: Asynchronous messages enable/disable support

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Sun, 24 Jan 2010 18:35:14 +0000
> Jamie Lokier <jamie@shareable.org> wrote:
>
>> Anthony Liguori wrote:
>> > >I'd like to be able to connect and be sure not to receive any async
>> > >messages, from simple scripts with simple output parsing.
>> > 
>> > You can't have simple output parsing with QMP.  You need a full JSON 
>> > stack.  The simplest script would be a python script that uses the 
>> > builtin json support.  Having async messages means that you'll have to 
>> > loop on recv in order make sure that the response is a command response 
>> > vs. an async message.  It's just a few lines more of code so I have a 
>> > hard time believing it's really a problem.
>> > 
>> > But what you probably want is a python QMP library and that would mean 
>> > you wouldn't need the few more lines of code.
>> 
>> You're right.  To be honest, parsing JSON can be done in a single Perl
>> regexp; a "full JSON stack" isn't much.
>> 
>> On that note, it'd be good if the end of a QMP message is framed with
>> something that can't appear inside the JSON value, without having to
>> parse the JSON incrementally on each partial read().  There are plenty
>> of short character sequences to choose from that can't appear.  Some
>> JSON parsers expect a whole well-formed expression or throw a parse
>> error - it's what people do over HTTP after all.  So you wait until
>> you think you've read a whole one, then pass it to the JSON parser.
>
>  The Monitor sends a CRLF sequence at the end of each line and I have
> maintained that behaivor for QMP, but it's hard to _guarantee_ that this
> sequence won't appear inside a json-string.

JSON requires control characters in strings to be escaped.  RFC 4627
section 2.5:

   A string begins and ends with quotation marks.  All Unicode
   characters may be placed within the quotation marks except for the
   characters that must be escaped: quotation mark, reverse solidus, and
   the control characters (U+0000 through U+001F).

>  In practice it doesn't, afaik.

If it doesn't, then it's not proper JSON, is it?

Re: [Qemu-devel] [PATCH 08/11] QMP: Asynchronous messages enable/disable support

[Luiz Capitulino]

On Mon, 25 Jan 2010 15:15:59 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Sun, 24 Jan 2010 18:35:14 +0000
> > Jamie Lokier <jamie@shareable.org> wrote:
> >
> >> Anthony Liguori wrote:
> >> > >I'd like to be able to connect and be sure not to receive any async
> >> > >messages, from simple scripts with simple output parsing.
> >> > 
> >> > You can't have simple output parsing with QMP.  You need a full JSON 
> >> > stack.  The simplest script would be a python script that uses the 
> >> > builtin json support.  Having async messages means that you'll have to 
> >> > loop on recv in order make sure that the response is a command response 
> >> > vs. an async message.  It's just a few lines more of code so I have a 
> >> > hard time believing it's really a problem.
> >> > 
> >> > But what you probably want is a python QMP library and that would mean 
> >> > you wouldn't need the few more lines of code.
> >> 
> >> You're right.  To be honest, parsing JSON can be done in a single Perl
> >> regexp; a "full JSON stack" isn't much.
> >> 
> >> On that note, it'd be good if the end of a QMP message is framed with
> >> something that can't appear inside the JSON value, without having to
> >> parse the JSON incrementally on each partial read().  There are plenty
> >> of short character sequences to choose from that can't appear.  Some
> >> JSON parsers expect a whole well-formed expression or throw a parse
> >> error - it's what people do over HTTP after all.  So you wait until
> >> you think you've read a whole one, then pass it to the JSON parser.
> >
> >  The Monitor sends a CRLF sequence at the end of each line and I have
> > maintained that behaivor for QMP, but it's hard to _guarantee_ that this
> > sequence won't appear inside a json-string.
> 
> JSON requires control characters in strings to be escaped.  RFC 4627
> section 2.5:

 Excellent.

> >  In practice it doesn't, afaik.
> 
> If it doesn't, then it's not proper JSON, is it?

 My 'doesn\'t' meant: no JSON string we currently generate has CR or
LF in them, as far as I can remember. But even if it happens it's
not a problem, as you clarified.

Re: [Qemu-devel] [PATCH 08/11] QMP: Asynchronous messages enable/disable support

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 22 Jan 2010 12:05:19 -0600
> Anthony Liguori <anthony@codemonkey.ws> wrote:
>
>> On 01/21/2010 03:09 PM, Luiz Capitulino wrote:
>> > This commit disables asynchronous messages by default and
>> > introduces two new QMP commands: async_msg_enable and
>> > async_msg_disable.
>> >
>> > Each QMP Monitor has its own set of asynchronous messages,
>> > so for example, if QEMU is run with two QMP Monitors async
>> > messages setup in one of them doesn't affect the other.
>> >
>> > To implement this design a bitmap is introduced to the
>> > Monitor struct, each async message is represented by one bit.
>> >
>> > Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>> >    
>> 
>> Ah, I see I was a little confused.
>> 
>> I'd suggest making async message masking an independent mechanism.  
>> Capabilities should strictly deal with protocol changes, not feature 
>> details.
>
>  To summarize (after a IRC talk): async messages are considered a
> capability and should be enabled during the negotiation phase but
> the masking of particular messages are not and can be done at
> any time after the negotiation phase.
>
>  I'm ok with that, Markus?

I agree with Anthony that async message masking doesn't really affect
the protocol proper.  We could pretend it does so we can let protocol
capability negotiation (which we need anyway) cover it.  But I'm
certainly fine with keeping it separate.

Whether we call it protocol or not, the question whether we should
permit changing the masks at any time is valid, I think.  Permitting it
adds a bit of conceptual complexity, as a command disabling reporting of
an event can race with the event.  But that's just giving clients some
more rope.  I'm fine with that.

Re: [Qemu-devel] [PATCH 08/11] QMP: Asynchronous messages enable/disable support

[Avi Kivity]

On 01/25/2010 04:29 PM, Markus Armbruster wrote:
>
> I agree with Anthony that async message masking doesn't really affect
> the protocol proper.  We could pretend it does so we can let protocol
> capability negotiation (which we need anyway) cover it.  But I'm
> certainly fine with keeping it separate.
>
> Whether we call it protocol or not, the question whether we should
> permit changing the masks at any time is valid, I think.  Permitting it
> adds a bit of conceptual complexity, as a command disabling reporting of
> an event can race with the event.  But that's just giving clients some
> more rope.  I'm fine with that.
>    

Without disagreeing with the rest (which means I'm just nit-picking), 
there's no race.  Once the command that disables an event report returns 
to the caller, the event can no longer be reported.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [RFC 00/11]: QMP feature negotiation support

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 01/21/2010 03:09 PM, Luiz Capitulino wrote:
>> """
>> {"QMP": {"capabilities": ["async messages"]}}
>>
>> { "execute": "query-qmp-mode" }
>> {"return": {"mode": "handshake"}}
>>
>> { "execute": "change", "arguments": { "device": "vnc", "target": "password", "arg": "1234" } }
>> {"error": {"class": "QMPInvalidModeCommad", "desc": "The issued command is invalid in this mode", "data": {}}}
>>
>> { "execute": "async_msg_enable", "arguments": { "name": "STOP" } }
>> {"return": {}}
>>    
>
> Maybe:
>
> enable-capability "async messages"
> disable-capability "async messages"
>
> I think that's a bit more obvious and it means that a client doesn't
> have to maintain a mapping of features -> enable functions.  It's also
> strange to use an enable command to disable something.

Agree on both counts.  But why two commands?  Why not simply "capability
NAME VALUE"?  Works even for non-boolean capabilities.  I'm not
predicting we'll need such capabilities.

Re: [Qemu-devel] [PATCH 08/11] QMP: Asynchronous messages enable/disable support

[Luiz Capitulino]

On Mon, 25 Jan 2010 16:33:02 +0200
Avi Kivity <avi@redhat.com> wrote:

> On 01/25/2010 04:29 PM, Markus Armbruster wrote:
> >
> > I agree with Anthony that async message masking doesn't really affect
> > the protocol proper.  We could pretend it does so we can let protocol
> > capability negotiation (which we need anyway) cover it.  But I'm
> > certainly fine with keeping it separate.
> >
> > Whether we call it protocol or not, the question whether we should
> > permit changing the masks at any time is valid, I think.  Permitting it
> > adds a bit of conceptual complexity, as a command disabling reporting of
> > an event can race with the event.  But that's just giving clients some
> > more rope.  I'm fine with that.
> >    
> 
> Without disagreeing with the rest (which means I'm just nit-picking), 
> there's no race.  Once the command that disables an event report returns 
> to the caller, the event can no longer be reported.

 I wouldn't call it a race, but if you don't want an event you'll have
to deal with it between mode change and masking.

 Not a big deal, only confirms that clients are required to know how to
ignore events, even if masking is available (which I'm not going to
introduce).

Re: [Qemu-devel] [RFC 00/11]: QMP feature negotiation support

[Luiz Capitulino]

On Mon, 25 Jan 2010 15:33:40 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Anthony Liguori <anthony@codemonkey.ws> writes:
> 
> > On 01/21/2010 03:09 PM, Luiz Capitulino wrote:
> >> """
> >> {"QMP": {"capabilities": ["async messages"]}}
> >>
> >> { "execute": "query-qmp-mode" }
> >> {"return": {"mode": "handshake"}}
> >>
> >> { "execute": "change", "arguments": { "device": "vnc", "target": "password", "arg": "1234" } }
> >> {"error": {"class": "QMPInvalidModeCommad", "desc": "The issued command is invalid in this mode", "data": {}}}
> >>
> >> { "execute": "async_msg_enable", "arguments": { "name": "STOP" } }
> >> {"return": {}}
> >>    
> >
> > Maybe:
> >
> > enable-capability "async messages"
> > disable-capability "async messages"
> >
> > I think that's a bit more obvious and it means that a client doesn't
> > have to maintain a mapping of features -> enable functions.  It's also
> > strange to use an enable command to disable something.
> 
> Agree on both counts.  But why two commands?  Why not simply "capability
> NAME VALUE"?  Works even for non-boolean capabilities.  I'm not
> predicting we'll need such capabilities.

 I slightly prefer two commands because that's probably how I'd
write it in a program (ie. two functions), also enabling/disabling
a group of features is a bit easier too, as we can use an array:

capability_enable [ "foo", "bar" ]

 Now, only one command is not terrible difficult, but we would
have to accept an array of objects, like:

[ { "name": "foo", "enabled": true }, { "name": "bar", "enabled": true } ]

Re: [Qemu-devel] [RFC 00/11]: QMP feature negotiation support

[Jamie Lokier]

Luiz Capitulino wrote:
> capability_enable [ "foo", "bar" ]
> 
>  Now, only one command is not terrible difficult, but we would
> have to accept an array of objects, like:
> 
> [ { "name": "foo", "enabled": true }, { "name": "bar", "enabled": true } ]

That looks like XML-itis.

Why not { "foo": true, "bar": true }?

-- Jamie

Re: [Qemu-devel] [RFC 00/11]: QMP feature negotiation support

[Luiz Capitulino]

On Tue, 26 Jan 2010 12:57:54 +0000
Jamie Lokier <jamie@shareable.org> wrote:

> Luiz Capitulino wrote:
> > capability_enable [ "foo", "bar" ]
> > 
> >  Now, only one command is not terrible difficult, but we would
> > have to accept an array of objects, like:
> > 
> > [ { "name": "foo", "enabled": true }, { "name": "bar", "enabled": true } ]
> 
> That looks like XML-itis.

 This is valid json, we already output data in this format and will
likely accept it in other commands.

> Why not { "foo": true, "bar": true }?

 Possible, but if we use a dict then I would prefer the previous format,
because it can be extended in a compatible way (while a single list and
yours don't).

Re: [Qemu-devel] [RFC 00/11]: QMP feature negotiation support

[Daniel P. Berrange]

On Tue, Jan 26, 2010 at 12:57:54PM +0000, Jamie Lokier wrote:
> Luiz Capitulino wrote:
> > capability_enable [ "foo", "bar" ]
> > 
> >  Now, only one command is not terrible difficult, but we would
> > have to accept an array of objects, like:
> > 
> > [ { "name": "foo", "enabled": true }, { "name": "bar", "enabled": true } ]
> 
> That looks like XML-itis.
> 
> Why not { "foo": true, "bar": true }?

It depends on whether we think we're going to need to add more metadata
beyond just the enabled/disabled status. If we did want to add a further
item against foo & bar, then having the array of hashes makes that 
extension easier becaue you add easily add more key/value pairs to
each.


Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [RFC 00/11]: QMP feature negotiation support

[Jamie Lokier]

Daniel P. Berrange wrote:
> On Tue, Jan 26, 2010 at 12:57:54PM +0000, Jamie Lokier wrote:
> > Luiz Capitulino wrote:
> > > capability_enable [ "foo", "bar" ]
> > > 
> > >  Now, only one command is not terrible difficult, but we would
> > > have to accept an array of objects, like:
> > > 
> > > [ { "name": "foo", "enabled": true }, { "name": "bar", "enabled": true } ]
> > 
> > That looks like XML-itis.
> > 
> > Why not { "foo": true, "bar": true }?
> 
> It depends on whether we think we're going to need to add more metadata
> beyond just the enabled/disabled status. If we did want to add a further
> item against foo & bar, then having the array of hashes makes that 
> extension easier becaue you add easily add more key/value pairs to
> each.

Sure, extensibility is good, and I personally don't care which
format/function are used.  Just wanted to question the padded
structure, because sometimes that style is done unintentially.

Look at the argument leading up here - Luiz says let's use separate,
non-extensible enable/disable commands taking a list, because if it
were a single command it'd be important to make it extensible.  Does
that make sense?  I don't understand that reasoning.

On that topic: In the regular monitor, commands are often extensible
because they take command-line-style options, and you can always add
more options.  What about QMP - are QMP commands all future-extensible
with options in a similar way?

-- Jamie

(ps. XML-itis: a tendancy to write
<element><name>tag</name><attribute><attr-name>name</attr-name><attr-value>value</attr-value></attribute></element>,
when <tag name=value/> would do).

Re: [Qemu-devel] [RFC 00/11]: QMP feature negotiation support

[Luiz Capitulino]

On Tue, 26 Jan 2010 15:57:46 +0000
Jamie Lokier <jamie@shareable.org> wrote:

> Daniel P. Berrange wrote:
> > On Tue, Jan 26, 2010 at 12:57:54PM +0000, Jamie Lokier wrote:
> > > Luiz Capitulino wrote:
> > > > capability_enable [ "foo", "bar" ]
> > > > 
> > > >  Now, only one command is not terrible difficult, but we would
> > > > have to accept an array of objects, like:
> > > > 
> > > > [ { "name": "foo", "enabled": true }, { "name": "bar", "enabled": true } ]
> > > 
> > > That looks like XML-itis.
> > > 
> > > Why not { "foo": true, "bar": true }?
> > 
> > It depends on whether we think we're going to need to add more metadata
> > beyond just the enabled/disabled status. If we did want to add a further
> > item against foo & bar, then having the array of hashes makes that 
> > extension easier becaue you add easily add more key/value pairs to
> > each.
> 
> Sure, extensibility is good, and I personally don't care which
> format/function are used.  Just wanted to question the padded
> structure, because sometimes that style is done unintentially.
> 
> Look at the argument leading up here - Luiz says let's use separate,
> non-extensible enable/disable commands taking a list, because if it
> were a single command it'd be important to make it extensible.  Does
> that make sense?  I don't understand that reasoning.

 I didn't consider extensibility in my first format, but we could also
have:

capability_enable [ { "name": "foo" }, { "name": "bar" } ]

> On that topic: In the regular monitor, commands are often extensible
> because they take command-line-style options, and you can always add
> more options.  What about QMP - are QMP commands all future-extensible
> with options in a similar way?

 Yes, command input is done through a json-object as does output.