[Qemu-devel] [PATCH 0/8]: QMP feature negotiation support

[Qemu-devel] [PATCH 0/8]: QMP feature negotiation support

[Luiz Capitulino]

 Feature negotiation allows clients to enable new QMP capabilities they
support and thus allows QMP to envolve in a compatible way.

 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, among other changes, adding
mode-oriented support to QMP, which comprehends the following:

o Two modes: handshake and operational
o 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

 Please, note that we don't have any capability yet. So, the most visable
change in this series is that now Clients must switch to operational mode to
be able to issue 'regular' commands.

 Session example:

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

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

{ "execute": "stop" }
{"error": {"class": "CommandNotFound", "desc": "The command stop has not been found", "data": {"name": "stop"}}}

{ "execute": "qmp_capability_enable", "arguments": { "name": "foobar" } }
{"error": {"class": "InvalidParameter", "desc": "Invalid parameter name", "data": {"name": "name"}}}

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

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

{ "execute": "stop" }
{"return": {}}

"""

[Qemu-devel] [PATCH 1/8] QMP: Initial mode-oriented support

[Luiz Capitulino]

In order to have 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

o Operational: regular Monitor operations, all handlers
(except the protocol configuration ones) are allowed

This commit does the following:

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 fb7c572..baae9d0 100644
--- a/monitor.c
+++ b/monitor.c
@@ -82,11 +82,16 @@ struct MonitorCompletionData {
     void (*user_print)(Monitor *mon, const QObject *data);
 };
 
+/* 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);
@@ -108,8 +113,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;
@@ -2471,6 +2482,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,
     },
@@ -2479,6 +2491,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,
     },
@@ -4293,6 +4306,7 @@ static void monitor_control_event(void *opaque, int event)
         QObject *data;
         Monitor *mon = opaque;
 
+        mon->mc->mode = QMODE_HANDSHAKE;
         json_message_parser_init(&mon->mc->parser, handle_qmp_command);
 
         data = qobject_from_jsonf("{ 'QMP': { 'capabilities': [] } }");
-- 
1.6.6

Re: [Qemu-devel] [PATCH 1/8] QMP: Initial mode-oriented support

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> In order to have 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
>
> o Operational: regular Monitor operations, all handlers
> (except the protocol configuration ones) are allowed
>
> This commit does the following:
>
> 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 fb7c572..baae9d0 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -82,11 +82,16 @@ struct MonitorCompletionData {
>      void (*user_print)(Monitor *mon, const QObject *data);
>  };
>  
> +/* Handler flags */
> +#define HANDLER_HANDSHAKE      0x01 /* allowed to run on handshake mode */
> +#define HANDLER_HANDSHAKE_ONLY 0x02 /* can ONLY run on handshake mode */
> +

A command's flags encode the set of modes in which this command may be
used.  The obvious way to encode a set of modes is to have an enable bit
for each mode.

Your choice of flags has the advantage that the value zero, which
doesn't need an initializer, means "operational mode only".

Fine with me.

>  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);
> @@ -108,8 +113,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;
> @@ -2471,6 +2482,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,
>      },
> @@ -2479,6 +2491,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,
>      },
> @@ -4293,6 +4306,7 @@ static void monitor_control_event(void *opaque, int event)
>          QObject *data;
>          Monitor *mon = opaque;
>  
> +        mon->mc->mode = QMODE_HANDSHAKE;
>          json_message_parser_init(&mon->mc->parser, handle_qmp_command);
>  
>          data = qobject_from_jsonf("{ 'QMP': { 'capabilities': [] } }");

[Qemu-devel] [PATCH 2/8] QMP: Introduce 'query-qmp-mode' command

[Luiz Capitulino]

Only valid in QMP and allowed to run on 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 baae9d0..3ced51d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -672,6 +672,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
@@ -2727,6 +2760,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 3/8] QError: Add QMP mode-oriented errors

[Luiz Capitulino]

Two new errors:

- QERR_QMP_INVALID_MODE_NAME
- QERR_QMP_INVALID_MODE_TRANSITION

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 6c2aba0..d01354d 100644
--- a/qerror.c
+++ b/qerror.c
@@ -113,6 +113,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_TRANSITION,
+        .desc      = "Invalid mode transition",
+    },
+    {
         .error_fmt = QERR_SET_PASSWD_FAILED,
         .desc      = "Could not set password",
     },
diff --git a/qerror.h b/qerror.h
index 57c5b97..41154b1 100644
--- a/qerror.h
+++ b/qerror.h
@@ -94,6 +94,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_TRANSITION \
+    "{ 'class': 'QMPInvalidModeTransition', 'data': {} }"
+
 #define QERR_SET_PASSWD_FAILED \
     "{ 'class': 'SetPasswdFailed', 'data': {} }"
 
-- 
1.6.6

Re: [Qemu-devel] [PATCH 3/8] QError: Add QMP mode-oriented errors

[Anthony Liguori]

On 01/28/2010 07:42 AM, Luiz Capitulino wrote:
> Two new errors:
>
> - QERR_QMP_INVALID_MODE_NAME
> - QERR_QMP_INVALID_MODE_TRANSITION
>
> 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 6c2aba0..d01354d 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -113,6 +113,14 @@ static const QErrorStringTable qerror_table[] = {
>           .desc      = "Bad QMP input object",
>       },
>       {
> +        .error_fmt = QERR_QMP_INVALID_MODE_NAME,
> +        .desc      = "Mode name %(name) is invalid",
> +    },
>    

This is basically, invalid parameter, no?

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 3/8] QError: Add QMP mode-oriented errors

[Luiz Capitulino]

On Thu, 28 Jan 2010 16:49:00 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 01/28/2010 07:42 AM, Luiz Capitulino wrote:
> > Two new errors:
> >
> > - QERR_QMP_INVALID_MODE_NAME
> > - QERR_QMP_INVALID_MODE_TRANSITION
> >
> > 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 6c2aba0..d01354d 100644
> > --- a/qerror.c
> > +++ b/qerror.c
> > @@ -113,6 +113,14 @@ static const QErrorStringTable qerror_table[] = {
> >           .desc      = "Bad QMP input object",
> >       },
> >       {
> > +        .error_fmt = QERR_QMP_INVALID_MODE_NAME,
> > +        .desc      = "Mode name %(name) is invalid",
> > +    },
> >    
> 
> This is basically, invalid parameter, no?

 Yeah.

Re: [Qemu-devel] [PATCH 3/8] QError: Add QMP mode-oriented errors

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 28 Jan 2010 16:49:00 -0600
> Anthony Liguori <anthony@codemonkey.ws> wrote:
>
>> On 01/28/2010 07:42 AM, Luiz Capitulino wrote:
>> > Two new errors:
>> >
>> > - QERR_QMP_INVALID_MODE_NAME
>> > - QERR_QMP_INVALID_MODE_TRANSITION
>> >
>> > 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 6c2aba0..d01354d 100644
>> > --- a/qerror.c
>> > +++ b/qerror.c
>> > @@ -113,6 +113,14 @@ static const QErrorStringTable qerror_table[] = {
>> >           .desc      = "Bad QMP input object",
>> >       },
>> >       {
>> > +        .error_fmt = QERR_QMP_INVALID_MODE_NAME,
>> > +        .desc      = "Mode name %(name) is invalid",
>> > +    },
>> >    
>> 
>> This is basically, invalid parameter, no?
>
>  Yeah.

Then let's use that.

[Qemu-devel] [PATCH 4/8] 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       |   26 ++++++++++++++++++++++++++
 qemu-monitor.hx |   15 +++++++++++++++
 2 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index 3ced51d..f6dd64d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -704,6 +704,32 @@ 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");
+
+    /*
+     * Check is simple, as this function can only run in
+     * handshake mode.
+     */
+
+    if (!strcmp(mode, "operational")) {
+        mon->mc->mode = QMODE_OPERATIONAL;
+    } else if (!strcmp(mode,  "handshake")) {
+        /* only handshake -> operational is allowed */
+        qemu_error_new(QERR_QMP_INVALID_MODE_TRANSITION);
+    } 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 b51bb47..29155ce 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -1064,6 +1064,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 mode",
+        .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

Re: [Qemu-devel] [PATCH 4/8] QMP: Introduce qmp_switch_mode command

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> 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       |   26 ++++++++++++++++++++++++++
>  qemu-monitor.hx |   15 +++++++++++++++
>  2 files changed, 41 insertions(+), 0 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 3ced51d..f6dd64d 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -704,6 +704,32 @@ 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;
> +    }

So the command "qmp_switch_mode" is available in the human monitor, but
does nothing?

What about a flag "QMP only?"

[...]

Re: [Qemu-devel] [PATCH 4/8] QMP: Introduce qmp_switch_mode command

[Luiz Capitulino]

On Mon, 01 Feb 2010 18:04:04 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > 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       |   26 ++++++++++++++++++++++++++
> >  qemu-monitor.hx |   15 +++++++++++++++
> >  2 files changed, 41 insertions(+), 0 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 3ced51d..f6dd64d 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -704,6 +704,32 @@ 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;
> > +    }
> 
> So the command "qmp_switch_mode" is available in the human monitor, but
> does nothing?
> 
> What about a flag "QMP only?"

 Seems reasonable, I only have to check if the work it requires is
related to this series.

[Qemu-devel] [PATCH 5/8] QMP: Introduce qmp_capability_enable/disable

[Luiz Capitulino]

As we don't have any capabilities yet, they always return an
InvalidParameter error.

Please, also note that:

o They don't accept an json-array yet, because this would require
not so simple changes and it's not useful right now

o We will need more infrastructure to have capabilities per Monitor,
which is not going to be done now as we don't need this yet either

Usage example:

{ "execute": "qmp_capability_enable", "arguments": { "name": "foo" } }

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

diff --git a/monitor.c b/monitor.c
index f6dd64d..9070a0c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -730,6 +730,20 @@ static void do_qmp_switch_mode(Monitor *mon, const QDict *qdict,
     }
 }
 
+static void do_qmp_capability_enable(Monitor *mon, const QDict *qdict,
+                                     QObject **ret_data)
+{
+    /* QMP doesn't have any capabilities yet */
+    qemu_error_new(QERR_INVALID_PARAMETER, "name");
+}
+
+static void do_qmp_capability_disable(Monitor *mon, const QDict *qdict,
+                                      QObject **ret_data)
+{
+    /* QMP doesn't have any capabilities yet */
+    qemu_error_new(QERR_INVALID_PARAMETER, "name");
+}
+
 /**
  * do_info_commands(): List QMP available commands
  *
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 29155ce..8905986 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -1079,6 +1079,36 @@ STEXI
 Switch QMP to @var{mode}
 ETEXI
 
+    {
+        .name       = "qmp_capability_enable",
+        .args_type  = "name:s",
+        .params     = "name",
+        .help       = "enable a QMP capability",
+        .flags      = HANDLER_HANDSHAKE_ONLY,
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_qmp_capability_enable,
+    },
+
+STEXI
+@item qmp_capability_enable @var{name}
+Enable QMP capability @{name}
+ETEXI
+
+    {
+        .name       = "qmp_capability_disable",
+        .args_type  = "name:s",
+        .params     = "name",
+        .help       = "disable a QMP capability",
+        .flags      = HANDLER_HANDSHAKE_ONLY,
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_qmp_capability_disable,
+    },
+
+STEXI
+@item qmp_capability_disable @var{name}
+Disable QMP capability @{name}
+ETEXI
+
 STEXI
 @end table
 ETEXI
-- 
1.6.6

[Qemu-devel] [PATCH 6/8] 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 9070a0c..5851b6d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -538,6 +538,19 @@ static void user_async_info_handler(Monitor *mon, const mon_cmd_t *cmd)
     }
 }
 
+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;
@@ -548,12 +561,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 7/8] QMP: Enable feature negotiation support

[Luiz Capitulino]

It's done by starting QMP in handshake mode, where (apart from a
few exceptions) only commands to query/enable/disable protocol
capabilities are allowed. Asynchronous messages are also disabled
in this mode.

Clients can change to the operational mode (where capabilities'
changes take effect and most commands are allowed) at any time.

When the issued command is not allowed to be executed on the
running mode, QMP will return the CommandNotFound error, as
suggested by Markus.

All these changes should have no effect in the user Monitor.

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

diff --git a/monitor.c b/monitor.c
index 5851b6d..28a8c32 100644
--- a/monitor.c
+++ b/monitor.c
@@ -403,10 +403,12 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
     }
 
     QLIST_FOREACH(mon, &mon_list, entry) {
-        if (monitor_ctrl_mode(mon)) {
+        if (monitor_ctrl_mode(mon) &&
+            mon->mc->mode == QMODE_OPERATIONAL) {
             monitor_json_emitter(mon, QOBJECT(qmp));
         }
     }
+
     QDECREF(qmp);
 }
 
@@ -4242,12 +4244,34 @@ static int monitor_check_qmp_args(const mon_cmd_t *cmd, QDict *args)
     return err;
 }
 
+static int qmp_invalid_mode(const Monitor *mon, unsigned int cmd_flags)
+{
+    switch (mon->mc->mode) {
+        case QMODE_OPERATIONAL:
+            if (cmd_flags & HANDLER_HANDSHAKE_ONLY) {
+                return 1;
+            }
+            break;
+        case QMODE_HANDSHAKE:
+            if (!((cmd_flags & HANDLER_HANDSHAKE) ||
+                (cmd_flags & HANDLER_HANDSHAKE_ONLY))) {
+                return 1;
+            }
+            break;
+        default:
+            abort();
+    }
+
+    return 0;
+}
+
 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;
 
@@ -4289,6 +4313,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 if 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));
@@ -4298,6 +4331,12 @@ 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_invalid_mode(mon, cmd_flags)) {
+        qemu_error_new(QERR_COMMAND_NOT_FOUND, cmd_name);
+        goto err_input;
     }
 
     obj = qdict_get(input, "arguments");
-- 
1.6.6

[Qemu-devel] [PATCH 8/8] QMP: spec: Feature negotiation related changes

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 QMP/qmp-spec.txt |   60 ++++++++++++++++++++++++++++++++++-------------------
 1 files changed, 38 insertions(+), 22 deletions(-)

diff --git a/QMP/qmp-spec.txt b/QMP/qmp-spec.txt
index 56f388c..5aea09d 100644
--- a/QMP/qmp-spec.txt
+++ b/QMP/qmp-spec.txt
@@ -39,14 +39,20 @@ they can be in ANY order, thus no particular order should be assumed.
 
 2.1.2 All json-objects members are mandatory when not specified otherwise
 
-2.2 Server Greeting
--------------------
+2.2 Server Modes
+----------------
 
-Right when connected the Server will issue a greeting message, which signals
-that the connection has been successfully established and that the Server is
-waiting for commands.
+The Server supports two modes of operation: handshake and operational.
 
-The format is:
+2.2.1 Handshake mode
+--------------------
+
+When a Client establishes a connection to the Server, the greeting message
+is issued and the Server is in handshake mode. In this mode only a subset of
+the Server's commands are available, most of them deals exclusively with
+capabilities negotiation.
+
+The format of the greeting message is:
 
 { "QMP": { "capabilities": json-array } }
 
@@ -55,6 +61,17 @@ The format is:
 - The "capabilities" member specify the availability of features beyond the
   baseline specification
 
+Clients should use the handshake mode to enable protocol features they support.
+
+2.2.2 Operational mode
+----------------------
+
+In operational mode the full set of Server's commands are available, the only
+exceptions are commands which deal exclusively with capabilities negotiation.
+
+Clients can switch from handshake mode to operational mode at any time,
+section 3.6 contains an example.
+
 2.3 Issuing Commands
 --------------------
 
@@ -123,8 +140,9 @@ if provided by the client.
 2.5 Asynchronous events
 -----------------------
 
-As a result of state changes, the Server may send messages unilaterally
-to the Client at any time. They are called 'asynchronous events'.
+When in operational mode, as a result of state changes, the Server may send
+messages unilaterally to the Client at any time. Theses messages are called
+'asynchronous events'.
 
 The format is:
 
@@ -179,25 +197,23 @@ S: {"error": {"class": "JSONParsing", "desc": "Invalid JSON syntax", "data":
 S: {"timestamp": {"seconds": 1258551470, "microseconds": 802384}, "event":
 "POWERDOWN"}
 
+3.6 Mode switch
+----------------
+
+C: { "execute": "qmp_switch_mode", "arguments": { "mode": "operational" } }
+S: {"return": {}}
+
 4. Compatibility Considerations
 --------------------------------
 
-In order to achieve maximum compatibility between versions, Clients must not 
-assume any particular:
+All protocol changes or new features which modify the protocol format in an
+incompatible way are disabled by default and will be listed in the capabilities
+array (section 2.2.1). Thus, Clients can enable new features they support at
+handshake mode.
+
+Additionally, Clients must not assume any particular:
 
 - Size of json-objects or length of json-arrays
 - Order of json-object members or json-array elements
 - Amount of errors generated by a command, that is, new errors can be added
   to any existing command in newer versions of the Server
-
-Additionally, Clients should always:
-
-- Check the capabilities json-array at connection time
-- Check the availability of commands with 'query-commands' before issuing them
-
-5. Recommendations to Client implementors
------------------------------------------
-
-5.1 The Server should be always started in pause mode, thus the Client is
-    able to perform any setup procedure without the risk of race conditions
-    and related problems
-- 
1.6.6

Re: [Qemu-devel] [PATCH 0/8]: QMP feature negotiation support

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

>  Feature negotiation allows clients to enable new QMP capabilities they
> support and thus allows QMP to envolve in a compatible way.
>
>  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.

Well, it becomes part of the protocol then.  But I understand what you
mean.

>  Feature negotiation is implemented by, among other changes, adding
> mode-oriented support to QMP, which comprehends the following:
>
> o Two modes: handshake and operational
> o 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
>
>  Please, note that we don't have any capability yet. So, the most visable
> change in this series is that now Clients must switch to operational mode to
> be able to issue 'regular' commands.
>
>  Session example:
>
> """
> {"QMP": {"capabilities": []}}
>
> { "execute": "query-qmp-mode" }
> {"return": {"mode": "handshake"}}
>
> { "execute": "stop" }
> {"error": {"class": "CommandNotFound", "desc": "The command stop has not been found", "data": {"name": "stop"}}}
>
> { "execute": "qmp_capability_enable", "arguments": { "name": "foobar" } }
> {"error": {"class": "InvalidParameter", "desc": "Invalid parameter name", "data": {"name": "name"}}}
>
> { "execute": "qmp_switch_mode", "arguments": { "mode": "operational" } }
> {"return": {}}
>
> { "execute": "query-qmp-mode" }
> {"return": {"mode": "operational"}}
>
> { "execute": "stop" }
> {"return": {}}
>
> """

I don't doubt your design does the job.  I just think it's overly
general.  I had something far more stupid in mind:

    client connects
    server -> client: version & capability offer (one message)
  again:
    client -> server: capability selection (one message)
    server -> client: either okay or error (one message)
    if error goto again
    connection is now ready for commands

No modes.  The distinct lack of generality is a design feature.

Re: [Qemu-devel] [PATCH 0/8]: QMP feature negotiation support

[Luiz Capitulino]

On Mon, 01 Feb 2010 18:08:27 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> >  Feature negotiation allows clients to enable new QMP capabilities they
> > support and thus allows QMP to envolve in a compatible way.
> >
> >  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.
> 
> Well, it becomes part of the protocol then.  But I understand what you
> mean.
> 
> >  Feature negotiation is implemented by, among other changes, adding
> > mode-oriented support to QMP, which comprehends the following:
> >
> > o Two modes: handshake and operational
> > o 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
> >
> >  Please, note that we don't have any capability yet. So, the most visable
> > change in this series is that now Clients must switch to operational mode to
> > be able to issue 'regular' commands.
> >
> >  Session example:
> >
> > """
> > {"QMP": {"capabilities": []}}
> >
> > { "execute": "query-qmp-mode" }
> > {"return": {"mode": "handshake"}}
> >
> > { "execute": "stop" }
> > {"error": {"class": "CommandNotFound", "desc": "The command stop has not been found", "data": {"name": "stop"}}}
> >
> > { "execute": "qmp_capability_enable", "arguments": { "name": "foobar" } }
> > {"error": {"class": "InvalidParameter", "desc": "Invalid parameter name", "data": {"name": "name"}}}
> >
> > { "execute": "qmp_switch_mode", "arguments": { "mode": "operational" } }
> > {"return": {}}
> >
> > { "execute": "query-qmp-mode" }
> > {"return": {"mode": "operational"}}
> >
> > { "execute": "stop" }
> > {"return": {}}
> >
> > """
> 
> I don't doubt your design does the job.  I just think it's overly
> general.  I had something far more stupid in mind:
> 
>     client connects
>     server -> client: version & capability offer (one message)
>   again:
>     client -> server: capability selection (one message)
>     server -> client: either okay or error (one message)
>     if error goto again
>     connection is now ready for commands
> 
> No modes.  The distinct lack of generality is a design feature.

 I like the simplicity and if we were allowed to change later I'd
do it.

 The question is if we will ever want features to be _configured_
before the protocol is operational. In this case we'd need to
pass feature arguments through the capability selection command,
which will get ugly and hard to use/understand.

 Mode oriented support doesn't have this limitation. Maybe we
won't never really use it, but it's safer.

Re: [Qemu-devel] [PATCH 0/8]: QMP feature negotiation support

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Mon, 01 Feb 2010 18:08:27 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> >  Feature negotiation allows clients to enable new QMP capabilities they
>> > support and thus allows QMP to envolve in a compatible way.
>> >
>> >  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.
>> 
>> Well, it becomes part of the protocol then.  But I understand what you
>> mean.
>> 
>> >  Feature negotiation is implemented by, among other changes, adding
>> > mode-oriented support to QMP, which comprehends the following:
>> >
>> > o Two modes: handshake and operational
>> > o 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
>> >
>> >  Please, note that we don't have any capability yet. So, the most visable
>> > change in this series is that now Clients must switch to operational mode to
>> > be able to issue 'regular' commands.
>> >
>> >  Session example:
>> >
>> > """
>> > {"QMP": {"capabilities": []}}
>> >
>> > { "execute": "query-qmp-mode" }
>> > {"return": {"mode": "handshake"}}
>> >
>> > { "execute": "stop" }
>> > {"error": {"class": "CommandNotFound", "desc": "The command stop has not been found", "data": {"name": "stop"}}}
>> >
>> > { "execute": "qmp_capability_enable", "arguments": { "name": "foobar" } }
>> > {"error": {"class": "InvalidParameter", "desc": "Invalid parameter name", "data": {"name": "name"}}}
>> >
>> > { "execute": "qmp_switch_mode", "arguments": { "mode": "operational" } }
>> > {"return": {}}
>> >
>> > { "execute": "query-qmp-mode" }
>> > {"return": {"mode": "operational"}}
>> >
>> > { "execute": "stop" }
>> > {"return": {}}
>> >
>> > """
>> 
>> I don't doubt your design does the job.  I just think it's overly
>> general.  I had something far more stupid in mind:
>> 
>>     client connects
>>     server -> client: version & capability offer (one message)
>>   again:
>>     client -> server: capability selection (one message)
>>     server -> client: either okay or error (one message)
>>     if error goto again
>>     connection is now ready for commands
>> 
>> No modes.  The distinct lack of generality is a design feature.
>
>  I like the simplicity and if we were allowed to change later I'd
> do it.
>
>  The question is if we will ever want features to be _configured_
> before the protocol is operational. In this case we'd need to
> pass feature arguments through the capability selection command,
> which will get ugly and hard to use/understand.
>
>  Mode oriented support doesn't have this limitation. Maybe we
> won't never really use it, but it's safer.

Capability selection could be done as an object where the name/value
pairs are capability/argument.  If you need multiple arguments for a
capability, make the capability's value an object.

If we need more *protocol* configuration than that, then we've grown a
few bells & whistles to many, I'd say.

Re: [Qemu-devel] [PATCH 0/8]: QMP feature negotiation support

[Luiz Capitulino]

On Mon, 01 Feb 2010 20:37:41 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Mon, 01 Feb 2010 18:08:27 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> >  Feature negotiation allows clients to enable new QMP capabilities they
> >> > support and thus allows QMP to envolve in a compatible way.
> >> >
> >> >  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.
> >> 
> >> Well, it becomes part of the protocol then.  But I understand what you
> >> mean.
> >> 
> >> >  Feature negotiation is implemented by, among other changes, adding
> >> > mode-oriented support to QMP, which comprehends the following:
> >> >
> >> > o Two modes: handshake and operational
> >> > o 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
> >> >
> >> >  Please, note that we don't have any capability yet. So, the most visable
> >> > change in this series is that now Clients must switch to operational mode to
> >> > be able to issue 'regular' commands.
> >> >
> >> >  Session example:
> >> >
> >> > """
> >> > {"QMP": {"capabilities": []}}
> >> >
> >> > { "execute": "query-qmp-mode" }
> >> > {"return": {"mode": "handshake"}}
> >> >
> >> > { "execute": "stop" }
> >> > {"error": {"class": "CommandNotFound", "desc": "The command stop has not been found", "data": {"name": "stop"}}}
> >> >
> >> > { "execute": "qmp_capability_enable", "arguments": { "name": "foobar" } }
> >> > {"error": {"class": "InvalidParameter", "desc": "Invalid parameter name", "data": {"name": "name"}}}
> >> >
> >> > { "execute": "qmp_switch_mode", "arguments": { "mode": "operational" } }
> >> > {"return": {}}
> >> >
> >> > { "execute": "query-qmp-mode" }
> >> > {"return": {"mode": "operational"}}
> >> >
> >> > { "execute": "stop" }
> >> > {"return": {}}
> >> >
> >> > """
> >> 
> >> I don't doubt your design does the job.  I just think it's overly
> >> general.  I had something far more stupid in mind:
> >> 
> >>     client connects
> >>     server -> client: version & capability offer (one message)
> >>   again:
> >>     client -> server: capability selection (one message)
> >>     server -> client: either okay or error (one message)
> >>     if error goto again
> >>     connection is now ready for commands
> >> 
> >> No modes.  The distinct lack of generality is a design feature.
> >
> >  I like the simplicity and if we were allowed to change later I'd
> > do it.
> >
> >  The question is if we will ever want features to be _configured_
> > before the protocol is operational. In this case we'd need to
> > pass feature arguments through the capability selection command,
> > which will get ugly and hard to use/understand.
> >
> >  Mode oriented support doesn't have this limitation. Maybe we
> > won't never really use it, but it's safer.
> 
> Capability selection could be done as an object where the name/value
> pairs are capability/argument.  If you need multiple arguments for a
> capability, make the capability's value an object.

 That's exactly what seems complicated to me, because besides performing
two functions (enable/configure) some feature setup could require
more commands to be done in a clear way.

 The async messages setup in the previous series was an example of this.

 As said we might never use this, but I wouldn't like to regret later.

Re: [Qemu-devel] [PATCH 0/8]: QMP feature negotiation support

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Mon, 01 Feb 2010 20:37:41 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Mon, 01 Feb 2010 18:08:27 +0100
>> > Markus Armbruster <armbru@redhat.com> wrote:
[...]
>> >> I don't doubt your design does the job.  I just think it's overly
>> >> general.  I had something far more stupid in mind:
>> >> 
>> >>     client connects
>> >>     server -> client: version & capability offer (one message)
>> >>   again:
>> >>     client -> server: capability selection (one message)
>> >>     server -> client: either okay or error (one message)
>> >>     if error goto again
>> >>     connection is now ready for commands
>> >> 
>> >> No modes.  The distinct lack of generality is a design feature.
>> >
>> >  I like the simplicity and if we were allowed to change later I'd
>> > do it.
>> >
>> >  The question is if we will ever want features to be _configured_
>> > before the protocol is operational. In this case we'd need to
>> > pass feature arguments through the capability selection command,
>> > which will get ugly and hard to use/understand.
>> >
>> >  Mode oriented support doesn't have this limitation. Maybe we
>> > won't never really use it, but it's safer.
>> 
>> Capability selection could be done as an object where the name/value
>> pairs are capability/argument.  If you need multiple arguments for a
>> capability, make the capability's value an object.
>
>  That's exactly what seems complicated to me, because besides performing
> two functions (enable/configure) some feature setup could require
> more commands to be done in a clear way.

What do you mean by "feature setup"?  And how does it go beyond setting
a bunch of parameters?

>  The async messages setup in the previous series was an example of this.

I don't remember the details.  Could you summarize?

>  As said we might never use this, but I wouldn't like to regret later.

A somewhat plausible example for how it could be needed would help.

Re: [Qemu-devel] [PATCH 0/8]: QMP feature negotiation support

[Luiz Capitulino]

On Tue, 02 Feb 2010 09:03:32 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Mon, 01 Feb 2010 20:37:41 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > On Mon, 01 Feb 2010 18:08:27 +0100
> >> > Markus Armbruster <armbru@redhat.com> wrote:
> [...]
> >> >> I don't doubt your design does the job.  I just think it's overly
> >> >> general.  I had something far more stupid in mind:
> >> >> 
> >> >>     client connects
> >> >>     server -> client: version & capability offer (one message)
> >> >>   again:
> >> >>     client -> server: capability selection (one message)
> >> >>     server -> client: either okay or error (one message)
> >> >>     if error goto again
> >> >>     connection is now ready for commands
> >> >> 
> >> >> No modes.  The distinct lack of generality is a design feature.
> >> >
> >> >  I like the simplicity and if we were allowed to change later I'd
> >> > do it.
> >> >
> >> >  The question is if we will ever want features to be _configured_
> >> > before the protocol is operational. In this case we'd need to
> >> > pass feature arguments through the capability selection command,
> >> > which will get ugly and hard to use/understand.
> >> >
> >> >  Mode oriented support doesn't have this limitation. Maybe we
> >> > won't never really use it, but it's safer.
> >> 
> >> Capability selection could be done as an object where the name/value
> >> pairs are capability/argument.  If you need multiple arguments for a
> >> capability, make the capability's value an object.
> >
> >  That's exactly what seems complicated to me, because besides performing
> > two functions (enable/configure) some feature setup could require
> > more commands to be done in a clear way.
> 
> What do you mean by "feature setup"?  And how does it go beyond setting
> a bunch of parameters?
> 
> >  The async messages setup in the previous series was an example of this.
> 
> I don't remember the details.  Could you summarize?

 Not the best example since we agreed async messages setup could be done
in operational mode, but in case other features will require it:

1. The async message feature _and_ each async message were disabled by
   default
2. You could enable async message feature with capability_enable
3. Then, each message should be enabled separately with async_message_enable

 The use case here is: a feature requires to be configured before the
protocol is operational.

 It's possible to do this with a command like feature, but it'll get
bloated over time.

Re: [Qemu-devel] [PATCH 0/8]: QMP feature negotiation support

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Tue, 02 Feb 2010 09:03:32 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Mon, 01 Feb 2010 20:37:41 +0100
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> 
>> >> > On Mon, 01 Feb 2010 18:08:27 +0100
>> >> > Markus Armbruster <armbru@redhat.com> wrote:
>> [...]
>> >> >> I don't doubt your design does the job.  I just think it's overly
>> >> >> general.  I had something far more stupid in mind:
>> >> >> 
>> >> >>     client connects
>> >> >>     server -> client: version & capability offer (one message)
>> >> >>   again:
>> >> >>     client -> server: capability selection (one message)
>> >> >>     server -> client: either okay or error (one message)
>> >> >>     if error goto again
>> >> >>     connection is now ready for commands
>> >> >> 
>> >> >> No modes.  The distinct lack of generality is a design feature.
>> >> >
>> >> >  I like the simplicity and if we were allowed to change later I'd
>> >> > do it.
>> >> >
>> >> >  The question is if we will ever want features to be _configured_
>> >> > before the protocol is operational. In this case we'd need to
>> >> > pass feature arguments through the capability selection command,
>> >> > which will get ugly and hard to use/understand.
>> >> >
>> >> >  Mode oriented support doesn't have this limitation. Maybe we
>> >> > won't never really use it, but it's safer.
>> >> 
>> >> Capability selection could be done as an object where the name/value
>> >> pairs are capability/argument.  If you need multiple arguments for a
>> >> capability, make the capability's value an object.
>> >
>> >  That's exactly what seems complicated to me, because besides performing
>> > two functions (enable/configure) some feature setup could require
>> > more commands to be done in a clear way.
>> 
>> What do you mean by "feature setup"?  And how does it go beyond setting
>> a bunch of parameters?
>> 
>> >  The async messages setup in the previous series was an example of this.
>> 
>> I don't remember the details.  Could you summarize?
>
>  Not the best example since we agreed async messages setup could be done
> in operational mode, but in case other features will require it:
>
> 1. The async message feature _and_ each async message were disabled by
>    default
> 2. You could enable async message feature with capability_enable
> 3. Then, each message should be enabled separately with async_message_enable
>
>  The use case here is: a feature requires to be configured before the
> protocol is operational.

Okay, let's pretend for the sake of the argument that async message
enable/disable is core protocol, and thus needs to be controlled via
capabilities.

An obvious way is to have one capability for every enable/disable
switch.  The server's capability offer lists them all, and the client's
capability selection includes the one it wants.

What if this leads to dozens of capabilities?  It's a machine protocol,
and a machine can cope with sixty capabilities just as fine as with six.
Six hundred would be kind of ugly, though.

If we absolutely insist on controlling async messages with a single
capability, things get slightly more complex.  The capability now sports
an object value, with a member for each enable/disable switch.  The
client's capability client selection supplies such an object value.

If we're worried about discoverability, we can make the server's
capability offer include a description of each capability's value.

And now let's quit pretending, and remind ourselves that capabilities
are for variations of the core protocol.  Do we really expect the core
protocol to become so baroque that we'll need a full-blown configuration
mode?

>  It's possible to do this with a command like feature, but it'll get
> bloated over time.

I doubt it.

Re: [Qemu-devel] [PATCH 0/8]: QMP feature negotiation support

[Anthony Liguori]

On 02/01/2010 01:50 PM, Luiz Capitulino wrote:
>> Capability selection could be done as an object where the name/value
>> pairs are capability/argument.  If you need multiple arguments for a
>> capability, make the capability's value an object.
>>      
>   That's exactly what seems complicated to me, because besides performing
> two functions (enable/configure) some feature setup could require
> more commands to be done in a clear way.
>    

I think the way to do this would be:

server -> client: version & capabilities offer (including cap 
NEGOTIATE_FEATURES)
client -> server: capability selection (including cap NEGOTIATE_FEATURES)
server -> client: okay or error
(session is now in negotiation mode)
client -> server: configure features
server -> client: ack/nack
client -> server: change to command mode
server -> client: ack/nack

So Markus' proposal solves our immediate needs while making it possible 
to implement something more sophisticated IFF we end up needing it for 
some reason.  It's a little more chatty than Luiz's proposal but since 
there isn't an obvious need now, I think it's a reasonable trade off to 
make.

Regards,

Anthony Liguori