[Qemu-devel] [PATCH 0/1] Get the list of arguments from a QMP command

[Qemu-devel] [PATCH 0/1] Get the list of arguments from a QMP command

[Alberto Garcia]

Hello,

this is a follow-up to the comments from Eric Blake about my patches
to extend the block streaming API:

   https://lists.gnu.org/archive/html/qemu-devel/2015-02/msg04231.html

Since QEMU doesn't have an easy way to tell the arguments that a
particular QMP command supports, and it seems that it would be useful
for libvirt and possibly others, I decided to write a simple patch
that adds that feature.

This is not an attempt to implement full introspection, but rather a
subset for this use case. I anyway tried to design it so it's easy to
extend in the future with the rest of the information.

I added a new type, CommandArgumentInfo, which includes the name of an
argument and a boolean expressing whether it's optional or not.

     { 'type': 'CommandArgumentInfo',
       'data': {'name': 'str', 'optional': 'bool'} }

I did not include the type of the argument since it would complicate
the code and would require me to parse the json files in order to
include all the details. My understanding is that for this use case
what's important for libvirt is knowing whether the argument is
supported, not its type (which libvirt should already know). But
please correct me if I'm wrong.

A new command 'query-command-args' returns the list of all supported
arguments for a particular command:

     { 'command': 'query-command-args',
       'data': {'command': 'str' },
       'returns': ['CommandArgumentInfo'] }

Alternatively, the existing 'query-commands' can be extended to accept
an optional parameter that specifies whether to return the arguments
for each command. That list of arguments would be added to the
'CommandInfo' type:

     { 'command': 'query-commands',
       'data': {'*query-args': 'bool' },
       'returns': ['CommandInfo'] }

     { 'type': 'CommandInfo',
       'data': {'name': 'str', 'args': ['CommandArgumentInfo'] } }

I added both alternatives in this patch since I don't know what's the
most convenient one for libvirt, but of course either of them can be
removed. The amount of C code needed to have both compared to just one
is negligible, though.

Feedback is welcome.

Thanks,

Berto

Alberto Garcia (1):
  qmp: Add support for requesting the list of arguments from a command

 monitor.c        | 53 +++++++++++++++++++++++++++++++-
 qapi/common.json | 41 +++++++++++++++++++++++--
 qmp-commands.hx  | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 178 insertions(+), 9 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH 1/1] qmp: Add support for requesting the list of arguments from a command

[Alberto Garcia]

This patch adds the CommandArgumentInfo type that lists the details of
an argument that a QMP command can take. It currently includes its name
and whether it's optional or not, but it can be extended in the future
to include other information.

A new 'query-command-args' command returns a list of all supported
arguments from a particular command. Additionally, 'query-commands'
has a new, optional, 'query-args' argument that makes the return value
include the arguments for all commands.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 monitor.c        | 53 +++++++++++++++++++++++++++++++-
 qapi/common.json | 41 +++++++++++++++++++++++--
 qmp-commands.hx  | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 178 insertions(+), 9 deletions(-)

diff --git a/monitor.c b/monitor.c
index c3cc060..867a1cc 100644
--- a/monitor.c
+++ b/monitor.c
@@ -231,6 +231,8 @@ static const mon_cmd_t qmp_cmds[];
 Monitor *cur_mon;
 Monitor *default_mon;
 
+static const mon_cmd_t *qmp_find_cmd(const char *cmdname);
+
 static void monitor_command_cb(void *opaque, const char *cmdline,
                                void *readline_opaque);
 
@@ -963,7 +965,52 @@ static void do_info_help(Monitor *mon, const QDict *qdict)
     help_cmd(mon, "info");
 }
 
-CommandInfoList *qmp_query_commands(Error **errp)
+CommandArgumentInfoList *qmp_query_command_args(const char *command,
+                                                Error **errp)
+{
+    CommandArgumentInfoList *cmd_list = NULL;
+    const mon_cmd_t *cmd = qmp_find_cmd(command);
+    const char *arg;
+
+    if (cmd == NULL) {
+        error_setg(errp, "Command '%s' not found", command);
+        return NULL;
+    }
+
+    for (arg = cmd->args_type; arg && *arg; arg = index(arg, ',')) {
+        CommandArgumentInfoList *info;
+        const char *type = index(arg, ':');
+
+        /* Skip the comma before the argument name */
+        if (*arg == ',') {
+            arg++;
+        }
+
+        /* Skip the semicolon before the argument type */
+        if (type) {
+            type++;
+        }
+
+        /* Check if the string is malformed */
+        if (!type || !*type) {
+            g_assert_not_reached();
+            return cmd_list;
+        }
+
+        info = g_malloc0(sizeof(*info));
+        info->value = g_malloc0(sizeof(*info->value));
+        info->value->name = g_strndup(arg, type - arg - 1);
+        info->value->optional = (type[0] == '-' || type[1] == '?');
+
+        info->next = cmd_list;
+        cmd_list = info;
+    }
+
+    return cmd_list;
+}
+
+CommandInfoList *qmp_query_commands(bool has_query_args, bool query_args,
+                                    Error **errp)
 {
     CommandInfoList *info, *cmd_list = NULL;
     const mon_cmd_t *cmd;
@@ -973,6 +1020,10 @@ CommandInfoList *qmp_query_commands(Error **errp)
         info->value = g_malloc0(sizeof(*info->value));
         info->value->name = g_strdup(cmd->name);
 
+        if (has_query_args && query_args) {
+            info->value->args = qmp_query_command_args(cmd->name, errp);
+        }
+
         info->next = cmd_list;
         cmd_list = info;
     }
diff --git a/qapi/common.json b/qapi/common.json
index 63ef3b4..d812557 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -66,26 +66,63 @@
 { 'command': 'query-version', 'returns': 'VersionInfo' }
 
 ##
+# @CommandArgumentInfo:
+#
+# Information about an argument that a QMP command can take.
+#
+# @name: The argument name.
+#
+# @optional: Whether the argument is optional or not
+#
+# Since: 2.3
+##
+{ 'type': 'CommandArgumentInfo',
+  'data': {'name': 'str', 'optional': 'bool'} }
+
+##
 # @CommandInfo:
 #
 # Information about a QMP command
 #
 # @name: The command name
 #
+# @args: A list of @CommandArgumentInfo that the command can take (Since 2.3)
+#
 # Since: 0.14.0
 ##
-{ 'type': 'CommandInfo', 'data': {'name': 'str'} }
+{ 'type': 'CommandInfo',
+  'data': {'name': 'str', 'args': ['CommandArgumentInfo'] } }
 
 ##
 # @query-commands:
 #
 # Return a list of supported QMP commands by this server
 #
+# @query-args: #optional Whether to return the list of arguments
+#                        for that command. Defaults to false (Since 2.3)
+#
 # Returns: A list of @CommandInfo for all supported commands
 #
 # Since: 0.14.0
 ##
-{ 'command': 'query-commands', 'returns': ['CommandInfo'] }
+{ 'command': 'query-commands',
+  'data': {'*query-args': 'bool' },
+  'returns': ['CommandInfo'] }
+
+##
+# @query-command-args:
+#
+# Return the list of arguments that a QMP command can take
+#
+# @command: Name of the command to query
+#
+# Returns: A list of @CommandArgumentInfo for all supported arguments
+#
+# Since: 2.3
+##
+{ 'command': 'query-command-args',
+  'data': {'command': 'str' },
+  'returns': ['CommandArgumentInfo'] }
 
 ##
 # @OnOffAuto
diff --git a/qmp-commands.hx b/qmp-commands.hx
index a85d847..cab69f9 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1946,25 +1946,52 @@ query-commands
 List QMP available commands.
 
 Each command is represented by a json-object, the returned value is a json-array
-of all commands.
+of all commands, optionally including their arguments.
+
+Arguments:
+
+- "query-args": whether to return the arguments for each command.
+                Defaults to false (json-bool, optional)
 
 Each json-object contain:
 
 - "name": command's name (json-string)
+- "args": a json-array of all arguments for that command. It will be empty if
+          the list of arguments was not requested using "query-args".
+          Each argument contains:
+          - "name": name of the argument (json-string)
+          - "optional": whether the argument is optional or not (json-bool)
 
 Example:
 
--> { "execute": "query-commands" }
+-> { "execute": "query-commands", "arguments": { "query-args": true } }
 <- {
       "return":[
          {
-            "name":"query-balloon"
+            "name": "drive-backup",
+            "args": [
+               { "name": "sync", "optional": false }
+               { "name": "device", "optional": false },
+               { "name": "target", "optional": false },
+               { "name": "speed", "optional": true },
+               { "name": "mode", "optional": true },
+               { "name": "format", "optional": true },
+               { "name": "on-source-error", "optional": true },
+               { "name": "on-target-error", "optional": true },
+            ]
          },
          {
-            "name":"system_powerdown"
+            "name": "block-commit",
+            "args": [
+               { "name": "device", "optional": false }
+               { "name": "base", "optional": true },
+               { "name": "top", "optional": true },
+               { "name": "backing-file", "optional": true },
+               { "name": "speed", "optional": true },
+            ]
          }
       ]
-   }
+}
 
 Note: This example has been shortened as the real response is too long.
 
@@ -1972,11 +1999,65 @@ EQMP
 
     {
         .name       = "query-commands",
-        .args_type  = "",
+        .args_type  = "query-args:b?",
         .mhandler.cmd_new = qmp_marshal_input_query_commands,
     },
 
 SQMP
+query-command-args
+------------------
+
+List all arguments from a QMP command.
+
+Each argument is represented by a json-object, the returned value is a
+json-array of all arguments from a particular command.
+
+Arguments:
+
+- "command": the command name (json-string)
+
+Each json-object contain:
+
+- "name": the name of the argument (json-string)
+- "optional": whether that argument is optional or not (json-bool)
+
+Example:
+
+-> { "execute": "query-command-args", "arguments": { "command": "block-commit" } }
+<- {
+      "return":[
+         {
+            "name": "device",
+            "optional": false
+         },
+         {
+            "name": "base",
+            "optional": true
+         },
+         {
+            "name": "top",
+            "optional": true
+         },
+         {
+            "name": "backing-file",
+            "optional": true
+         },
+         {
+            "name": "speed",
+            "optional": true
+         }
+      ]
+}
+
+EQMP
+
+    {
+        .name       = "query-command-args",
+        .args_type  = "command:s",
+        .mhandler.cmd_new = qmp_marshal_input_query_command_args,
+    },
+
+SQMP
 query-events
 --------------
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH 0/1] Get the list of arguments from a QMP command

[Eric Blake]

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

On 02/24/2015 06:51 AM, Alberto Garcia wrote:
> Hello,
> 
> this is a follow-up to the comments from Eric Blake about my patches
> to extend the block streaming API:
> 
>    https://lists.gnu.org/archive/html/qemu-devel/2015-02/msg04231.html
> 
> Since QEMU doesn't have an easy way to tell the arguments that a
> particular QMP command supports, and it seems that it would be useful
> for libvirt and possibly others, I decided to write a simple patch
> that adds that feature.

This is potentially a step in the right direction for full
introspection, but I'm a bit worried that it's half-baked.  That is, it
only states whether an argument is present or not, but doesn't say what
type those arguments are, and most glaringly, doesn't tell me anything
about type changes, such as adding new enum values or new struct
members.  In other words, while this interface might help libvirt, it
won't solve all the qapi questions that libvirt has, and I'm worried
that committing this and then adding full introspection will burden us
with multiple implementations to keep running.

> 
> This is not an attempt to implement full introspection, but rather a
> subset for this use case. I anyway tried to design it so it's easy to
> extend in the future with the rest of the information.
> 
> I added a new type, CommandArgumentInfo, which includes the name of an
> argument and a boolean expressing whether it's optional or not.
> 
>      { 'type': 'CommandArgumentInfo',
>        'data': {'name': 'str', 'optional': 'bool'} }

At least this representation IS extensible - you could add another dict
member giving the argument's type at a later date, as needed.

> 
> I did not include the type of the argument since it would complicate
> the code and would require me to parse the json files in order to
> include all the details. My understanding is that for this use case
> what's important for libvirt is knowing whether the argument is
> supported, not its type (which libvirt should already know). But
> please correct me if I'm wrong.

Indeed, knowing that an argument exists is often more important that
knowing its type (since we try to keep the type backward-compatible,
even when changing from 'str' to an enum means that libvirt could see
two different type answers according to which version of qemu it is
talking to, but libvirt's behavior in managing that parameter doesn't
care which type was reported, only that the parameter exists).

> 
> A new command 'query-command-args' returns the list of all supported
> arguments for a particular command:
> 
>      { 'command': 'query-command-args',
>        'data': {'command': 'str' },
>        'returns': ['CommandArgumentInfo'] }

The 'command' argument be optional, where omitting it gives an array of
ALL commands in one QMP call.  Otherwise, returning an array doesn't
make as much sense if it will always be a one-element array because the
filtering was mandatory.

But do we need a new command?

> 
> Alternatively, the existing 'query-commands' can be extended to accept
> an optional parameter that specifies whether to return the arguments
> for each command. That list of arguments would be added to the
> 'CommandInfo' type:
> 
>      { 'command': 'query-commands',
>        'data': {'*query-args': 'bool' },
>        'returns': ['CommandInfo'] }
> 
>      { 'type': 'CommandInfo',
>        'data': {'name': 'str', 'args': ['CommandArgumentInfo'] } }

Indeed, you already anticipated my question - I think that extending the
existing 'query-commands' API is just as easy to do.

> 
> I added both alternatives in this patch since I don't know what's the
> most convenient one for libvirt, but of course either of them can be
> removed. The amount of C code needed to have both compared to just one
> is negligible, though.
> 
> Feedback is welcome.

I'm still thinking about the actual patch, and whether we want to commit
to this or just bite the bullet and go for full introspection.  At any
rate, it's a bit late for 2.3, so we have the full 2.4 cycle to get it
right.

> 
> Thanks,
> 
> Berto
> 
> Alberto Garcia (1):
>   qmp: Add support for requesting the list of arguments from a command
> 
>  monitor.c        | 53 +++++++++++++++++++++++++++++++-
>  qapi/common.json | 41 +++++++++++++++++++++++--
>  qmp-commands.hx  | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  3 files changed, 178 insertions(+), 9 deletions(-)
> 

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


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

Re: [Qemu-devel] [PATCH 0/1] Get the list of arguments from a QMP command

[Alberto Garcia]

On Fri, Mar 06, 2015 at 10:11:39AM -0700, Eric Blake wrote:

> I'm still thinking about the actual patch, and whether we want
> to commit to this or just bite the bullet and go for full
> introspection.  At any rate, it's a bit late for 2.3, so we have the
> full 2.4 cycle to get it right.

I understand your concerns. I can actually try to implement full
introspection support, but I would like to know what API you would
like. Something like this?

   'query-commands'
   'query-enums'
   'query-events'
   'query-types'
   'query-unions'

Berto

Re: [Qemu-devel] [PATCH 0/1] Get the list of arguments from a QMP command

[Markus Armbruster]

Cc'ing Amos, who grappled with this in the past.

Alberto Garcia <berto@igalia.com> writes:

> On Fri, Mar 06, 2015 at 10:11:39AM -0700, Eric Blake wrote:
>
>> I'm still thinking about the actual patch, and whether we want
>> to commit to this or just bite the bullet and go for full
>> introspection.  At any rate, it's a bit late for 2.3, so we have the
>> full 2.4 cycle to get it right.
>
> I understand your concerns. I can actually try to implement full
> introspection support, but I would like to know what API you would
> like. Something like this?
>
>    'query-commands'
>    'query-enums'
>    'query-events'
>    'query-types'
>    'query-unions'

Observe that we already have a machine-readable interface description:
the QAPI schema.  It's JSON, so we could have a query-schema return it
verbatim (minus the comments, of course).

You propose a separate query-FOO for each kind of thing in the schema:
command, event, the various types.  Not fundamentally different to a
single query-schema.  We can discuss which of the two approaches is
easier to use.  But first we need to address a number problems with the
exposed schema's syntax and semantics.

Introspection by exposing the QAPI schema makes schema syntax and
semantics an external interface.  Trouble is these are ill-defined.
We'd have to fix that (good idea anyway), and review the result with
compatible extensibility in mind.  Eric's stalled series "[PATCH v4
00/19] drop qapi nested structs" is a start:
https://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg04022.html

The QAPI schema describes a union of internal interfaces and the
external QMP interface.  Unfortunate, because it makes the external
interface hard to see.  Naturally, QMP introspection should only expose
the QMP interface.  So we'd have to extract that part.

Here's how I'd try to do that.  Treat the schema as a graph, its nodes
are the definitions of types, commands and events are nodes, and a use
of a type adds an edge from the node containing the use to the type.
The external interface is exactly the sub-graph reachable QMP command
and event nodes.

Related prior work:
Amos's [PATCH v4 0/5] QMP full introspection
https://lists.nongnu.org/archive/html/qemu-devel/2014-01/msg03013.html
and the versions leading up to it, including the RFC
https://lists.nongnu.org/archive/html/qemu-devel/2013-05/msg03160.html

Re: [Qemu-devel] [PATCH 0/1] Get the list of arguments from a QMP command

[Kevin Wolf]

Am 11.03.2015 um 11:21 hat Markus Armbruster geschrieben:
> Cc'ing Amos, who grappled with this in the past.
> 
> Alberto Garcia <berto@igalia.com> writes:
> 
> > On Fri, Mar 06, 2015 at 10:11:39AM -0700, Eric Blake wrote:
> >
> >> I'm still thinking about the actual patch, and whether we want
> >> to commit to this or just bite the bullet and go for full
> >> introspection.  At any rate, it's a bit late for 2.3, so we have the
> >> full 2.4 cycle to get it right.
> >
> > I understand your concerns. I can actually try to implement full
> > introspection support, but I would like to know what API you would
> > like. Something like this?
> >
> >    'query-commands'
> >    'query-enums'
> >    'query-events'
> >    'query-types'
> >    'query-unions'
> 
> Observe that we already have a machine-readable interface description:
> the QAPI schema.  It's JSON, so we could have a query-schema return it
> verbatim (minus the comments, of course).

It's an awful structure for an external interface.

Things like designating optional fields with *name are great for humans
because they allow concise definitions where each field is just one dict
entry instead of a whole struct, and this keeps things readable.

This is important for the schema as it doesn't only serve as the source
for the code generator, but is edited by humans and used as an API
documentation. So we want to keep that.

But I'm pretty sure we don't want to send it as part of an external API.

> You propose a separate query-FOO for each kind of thing in the schema:
> command, event, the various types.  Not fundamentally different to a
> single query-schema.  We can discuss which of the two approaches is
> easier to use.  But first we need to address a number problems with the
> exposed schema's syntax and semantics.
> 
> Introspection by exposing the QAPI schema makes schema syntax and
> semantics an external interface.  Trouble is these are ill-defined.
> We'd have to fix that (good idea anyway), and review the result with
> compatible extensibility in mind.  Eric's stalled series "[PATCH v4
> 00/19] drop qapi nested structs" is a start:
> https://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg04022.html

Cleanups on the surface won't change any of the points above. I'm not
convinced that the schema could be friendly for humans and suitable as
an external API at the same time.

> The QAPI schema describes a union of internal interfaces and the
> external QMP interface.  Unfortunate, because it makes the external
> interface hard to see.  Naturally, QMP introspection should only expose
> the QMP interface.  So we'd have to extract that part.
> 
> Here's how I'd try to do that.  Treat the schema as a graph, its nodes
> are the definitions of types, commands and events are nodes, and a use
> of a type adds an edge from the node containing the use to the type.
> The external interface is exactly the sub-graph reachable QMP command
> and event nodes.

Yes, that's how I would do it, too.

Kevin

Re: [Qemu-devel] [PATCH 0/1] Get the list of arguments from a QMP command

[Alberto Garcia]

On Wed, Mar 11, 2015 at 11:21:43AM +0100, Markus Armbruster wrote:

> > I can actually try to implement full introspection support, but I
> > would like to know what API you would like. Something like this?
> >
> >    'query-commands'
> >    'query-enums'
> >    'query-events'
> >    'query-types'
> >    'query-unions'
> 
> You propose a separate query-FOO for each kind of thing in the
> schema: command, event, the various types.  Not fundamentally
> different to a single query-schema.  We can discuss which of the two
> approaches is easier to use.

I don't think it makes much difference in terms of complexity
from the QEMU side. I proposed those because query-commands and
query-events already exist, so adding the missing ones seemed the most
straightforward solution to me.

But if query-schema is more convenient I don't have any problem with
that.

> Here's how I'd try to do that.  Treat the schema as a graph, its
> nodes are the definitions of types, commands and events are nodes,
> and a use of a type adds an edge from the node containing the use to
> the type.  The external interface is exactly the sub-graph reachable
> QMP command and event nodes.

Makes sense.

Berto

Re: [Qemu-devel] [PATCH 0/1] Get the list of arguments from a QMP command

[Markus Armbruster]

Alberto Garcia <berto@igalia.com> writes:

> On Wed, Mar 11, 2015 at 11:21:43AM +0100, Markus Armbruster wrote:
>
>> > I can actually try to implement full introspection support, but I
>> > would like to know what API you would like. Something like this?
>> >
>> >    'query-commands'
>> >    'query-enums'
>> >    'query-events'
>> >    'query-types'
>> >    'query-unions'
>> 
>> You propose a separate query-FOO for each kind of thing in the
>> schema: command, event, the various types.  Not fundamentally
>> different to a single query-schema.  We can discuss which of the two
>> approaches is easier to use.
>
> I don't think it makes much difference in terms of complexity
> from the QEMU side. I proposed those because query-commands and
> query-events already exist, so adding the missing ones seemed the most
> straightforward solution to me.

They only tell you what commands and events exist, but nothing about
their 'data' or 'returns'.

> But if query-schema is more convenient I don't have any problem with
> that.

The real work is processing the full schema into the externally visible
sub-schema.  Whether to split it into several parts for presentation is
the least of my worries.

I'm fine with with building up introspection step by step.  But I want
us to have a reasonable idea of the end result.  Without that,
incremental development together with our ABI promise is prone to
produce a tangled mess.  We already have enough of those :)

I feel a necessary early step is to actually define our schema language,
and separate syntactic sugar from its core.  Introspection wants the
core, but for schema development, a modest amount of sugar is
convenient.  Some of the current schema syntax needs to be redefined as
sugar for a suitable desugared form.  We've discussed this for "asterisk
means optional" already.  Eric's series brings us closer, it just needs
to be finished.

If we can't crack the whole problem in the next development cycle,
perhaps we can identify a minimally useful sub-schema that is unlikely
to be affected by the above work.  We could then expose that early, with
the idea of extending it later.

[...]

Re: [Qemu-devel] [PATCH 0/1] Get the list of arguments from a QMP command

[Kevin Wolf]

Am 11.03.2015 um 20:22 hat Markus Armbruster geschrieben:
> Alberto Garcia <berto@igalia.com> writes:
> 
> > On Wed, Mar 11, 2015 at 11:21:43AM +0100, Markus Armbruster wrote:
> >
> >> > I can actually try to implement full introspection support, but I
> >> > would like to know what API you would like. Something like this?
> >> >
> >> >    'query-commands'
> >> >    'query-enums'
> >> >    'query-events'
> >> >    'query-types'
> >> >    'query-unions'
> >> 
> >> You propose a separate query-FOO for each kind of thing in the
> >> schema: command, event, the various types.  Not fundamentally
> >> different to a single query-schema.  We can discuss which of the two
> >> approaches is easier to use.
> >
> > I don't think it makes much difference in terms of complexity
> > from the QEMU side. I proposed those because query-commands and
> > query-events already exist, so adding the missing ones seemed the most
> > straightforward solution to me.
> 
> They only tell you what commands and events exist, but nothing about
> their 'data' or 'returns'.

True, but luckily they were defined to return not just a list of
strings, but of CommandInfo dicts. So extending that seems to be the
natural option in order to get a coherent interface in the end.

> > But if query-schema is more convenient I don't have any problem with
> > that.
> 
> The real work is processing the full schema into the externally visible
> sub-schema.  Whether to split it into several parts for presentation is
> the least of my worries.
> 
> I'm fine with with building up introspection step by step.  But I want
> us to have a reasonable idea of the end result.  Without that,
> incremental development together with our ABI promise is prone to
> produce a tangled mess.  We already have enough of those :)

Okay. Let's try to define just enough to get a reasonable idea of the
end result.

Let's start with the commands that we will have, without looking at
the structure of their arguments and return values yet:

* query-commands, which returns a list of CommandInfos. This contains at
  least the name of each command. (Mandated by backwards compatibility
  requirements)

* query-events, which returns a list of EventInfos.  This contains at
  least the name of each event. (Mandated by backwards compatibility
  requirements)

* Something to query the full description of commands, including the
  type of their return value and name, type and optionality of their
  arguments.

  I propose combining this with query-commands by extending CommandInfo.

* Something to query QAPI types, including:

  - The fields of a complex type, including their name, type and
    optionality.

    I propose that this information be returned in a structure analogous
    to how arguments are described in query-commands, whatever that may
    be.

  - The possible values of an enum type

  - Union types are complex (sorry, my bad), we probably don't want to
    expose them the way they are in the schema. This needs more thought.

  The details of how each type is described, as well as the exact
  command to query it, aren't of importance for adding command argument
  querying and shouldn't affect it, so I'd leave that for later.

  What I do propose is that in order to maintain consistency with the
  already existing query-{commands,events}, we introduct a separate
  command for these. This may be a single 'query-types', but it could
  also be 'query-complex-types'/'query-enums'/'query-unions'.

  Unless you think it makes a difference for defining what we need now,
  I think we can leave this question open.

* Possibly something to query the fields of events, if we ever need
  that (I doubt it). If we do, in consistency with the above I propose
  extending EventInfo and exposing the information the same way as for
  command arguments and complex type fields.

Do you agree so far?

Anything else you want to get answered before you think you have a
reasonable idea of the end result on the high level?

If not, the next step would be to define the exact structure of each
command. I for one don't think that there is a need to define it for all
of the commands now, but you may think otherwise?

If so, putting together the schema for the query commands (without
implementing them yet) shouldn't be that hard and I'd be happy to do
that. Perhaps with the exception of union types, but if you think it's
important, I can propose something for them, too.

> I feel a necessary early step is to actually define our schema language,
> and separate syntactic sugar from its core.  Introspection wants the
> core, but for schema development, a modest amount of sugar is
> convenient.  Some of the current schema syntax needs to be redefined as
> sugar for a suitable desugared form.  We've discussed this for "asterisk
> means optional" already.  Eric's series brings us closer, it just needs
> to be finished.

Our schema language is an implementation detail, we shouldn't worry
about it too much in this discussion. As long as we agree that arguments
do have a name, a type and a boolean that describes whether they are
optional (and I don't think anyone would deny that), we can use that in
a future-proof external API.

> If we can't crack the whole problem in the next development cycle,
> perhaps we can identify a minimally useful sub-schema that is unlikely
> to be affected by the above work.  We could then expose that early, with
> the idea of extending it later.

We've done that for several years and we haven't cracked it. We need to
start doing something. Now.

Look at blockdev. We only started moving because we stopped discussing
and actually started hacking on it. And blockdev is much more complex
and we still haven't figured out all the details. In comparison with
blockdev, schema introspection doesn't look that hard any more.

Kevin

Re: [Qemu-devel] [PATCH 0/1] Get the list of arguments from a QMP command

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 11.03.2015 um 20:22 hat Markus Armbruster geschrieben:
>> Alberto Garcia <berto@igalia.com> writes:
>> 
>> > On Wed, Mar 11, 2015 at 11:21:43AM +0100, Markus Armbruster wrote:
>> >
>> >> > I can actually try to implement full introspection support, but I
>> >> > would like to know what API you would like. Something like this?
>> >> >
>> >> >    'query-commands'
>> >> >    'query-enums'
>> >> >    'query-events'
>> >> >    'query-types'
>> >> >    'query-unions'
>> >> 
>> >> You propose a separate query-FOO for each kind of thing in the
>> >> schema: command, event, the various types.  Not fundamentally
>> >> different to a single query-schema.  We can discuss which of the two
>> >> approaches is easier to use.
>> >
>> > I don't think it makes much difference in terms of complexity
>> > from the QEMU side. I proposed those because query-commands and
>> > query-events already exist, so adding the missing ones seemed the most
>> > straightforward solution to me.
>> 
>> They only tell you what commands and events exist, but nothing about
>> their 'data' or 'returns'.
>
> True, but luckily they were defined to return not just a list of
> strings, but of CommandInfo dicts. So extending that seems to be the
> natural option in order to get a coherent interface in the end.
>
>> > But if query-schema is more convenient I don't have any problem with
>> > that.
>> 
>> The real work is processing the full schema into the externally visible
>> sub-schema.  Whether to split it into several parts for presentation is
>> the least of my worries.
>> 
>> I'm fine with with building up introspection step by step.  But I want
>> us to have a reasonable idea of the end result.  Without that,
>> incremental development together with our ABI promise is prone to
>> produce a tangled mess.  We already have enough of those :)
>
> Okay. Let's try to define just enough to get a reasonable idea of the
> end result.
>
> Let's start with the commands that we will have, without looking at
> the structure of their arguments and return values yet:
>
> * query-commands, which returns a list of CommandInfos. This contains at
>   least the name of each command. (Mandated by backwards compatibility
>   requirements)
>
> * query-events, which returns a list of EventInfos.  This contains at
>   least the name of each event. (Mandated by backwards compatibility
>   requirements)
>
> * Something to query the full description of commands, including the
>   type of their return value and name, type and optionality of their
>   arguments.
>
>   I propose combining this with query-commands by extending CommandInfo.

Detail.  I want us to figure out what information we want to provide
first, how to encode it second, and how to fit it into the existing
introspection interfaces third.

That said, extending CommandInfo isn't unreasonable.  I'm happy to get
back to the idea when we're ready to discuss "third".

Let me make an attempt at "first".

QMP introspection answers reasonable questions about the QMP interface.

Proposition: introspection of a suitable subset of the QAPI schema
provides QMP introspection.

Rationale:

    The QAPI schema is (meant to be) the definition of the QMP interface
    (plus a internal interfaces, but we can ignore that).

    Therefore, introspection answers can be found in the QAPI schema.
    If the schema can't answer a reasonable question, it's a defect in
    the schema or the interface.

Example:

    "Does this version of QEMU support frobnicating over QMP?" is a
    reasonable question.

    If we provide a QMP command 'frobnicate', the schema can answer the
    question.

    Likewise if we add a 'frobnicate' parameter to an existing QMP
    command.

    But if we extend the set of acceptable values of an existing
    parameter, it can't.  You can view this as a defect in the schema
    language: it can't express acceptable values.  Or you can view it as
    a defect in the interface: new features shouldn't be shoehorned into
    existing commands that way.

What we need to provide is a suitable subset of the information in the
QAPI schema.

What information is in the QAPI schema?  This is a surprisingly hard
question, because the QAPI schema is poorly defined[1].

The general form of a QMP command is

    { "execute": COMMAND-NAME, "arguments": ARGS, "id": ID }

where COMMAND-NAME and ID are JSON strings, and ARGS is a JSON object.

The general form of a success response is

    { "return": VALUE, "id": ID }

where VALUE can be almost any JSON value (more below), and ID is the
command's ID.

Error responses aren't interesting here, because they're shared by all
commands.

The schema has the following information on commands:

* Name

  This declares an accepted COMMAND-NAME.

* Parameters

  Declare what ARGS are accepted with COMMAND-NAME, one of:

  - A set of names with properties

    The names specify the object's acceptable member names.

    A name's properties are type and whether the member is optional.

    With Eric's series applied, a name's type property can only be a
    type name, not an anonymous complex type specification.

    Since you started at the interesting rather than the basic end of
    the schema, we haven't discussed types, yet, but if we had, you'd
    see that this is exactly a complex type declaration.

    Aside: generalization to arbitrary rather than just complex type
    would be possible.  Arbitrary type is natural when you view commands
    as taking a single (usually complex) argument and produce a single
    result.  Flat union types could be handy.

    Currently, the schema syntax doesn't permit properties beyond type
    and optionalness, but Eric's series prepares the ground for
    extensibility.  We already discussed adding a default value
    property.

  - A name of a complex type

    Same as above, except the type is named rather than anonymous.

  - "Anything"

    Any ARGS are accepted.

* Return value

  Declare what VALUE can be produced, one of:

  - JSON object: a set of names with properties or a name of a complex
    type

    Declares possible objects just like we for parameters anove.

    Can't say offhand whether union types are already possible here.

  - JSON array of a single type: a type name

  - JSON number, string or boolean: a built-in type, or the name of an
    enum type

Now we can discuss what subset to provide at first.  My opening bid is
"all of it", because it's really not all that much.

> * Something to query QAPI types, including:
>
>   - The fields of a complex type, including their name, type and
>     optionality.
>
>     I propose that this information be returned in a structure analogous
>     to how arguments are described in query-commands, whatever that may
>     be.
>
>   - The possible values of an enum type
>
>   - Union types are complex (sorry, my bad), we probably don't want to
>     expose them the way they are in the schema. This needs more thought.

Another thing to think through is base types.  Do we want to expose
them, or do we want to flatten them away?

>   The details of how each type is described, as well as the exact
>   command to query it, aren't of importance for adding command argument
>   querying and shouldn't affect it, so I'd leave that for later.

If we chicken out of introspecting types, we need to expand type names
into their definitions.  Remember, a command's parameters can be defined
as a type name, and its return value, too.

I'm not sure this would reduce complexity much short term.  I suspect
it'll increase it for the complete solution.

>   What I do propose is that in order to maintain consistency with the
>   already existing query-{commands,events}, we introduct a separate
>   command for these. This may be a single 'query-types', but it could
>   also be 'query-complex-types'/'query-enums'/'query-unions'.

Again, I'm not ready to discuss commands just yet.

>   Unless you think it makes a difference for defining what we need now,
>   I think we can leave this question open.
>
> * Possibly something to query the fields of events, if we ever need
>   that (I doubt it). If we do, in consistency with the above I propose
>   extending EventInfo and exposing the information the same way as for
>   command arguments and complex type fields.
>
> Do you agree so far?
>
> Anything else you want to get answered before you think you have a
> reasonable idea of the end result on the high level?

Yes: I want an analysis of types and events similar to my above analysis
of commands.  I'll do it myself.  Just not today, I'm running rather
late already.

> If not, the next step would be to define the exact structure of each
> command. I for one don't think that there is a need to define it for all
> of the commands now, but you may think otherwise?
>
> If so, putting together the schema for the query commands (without
> implementing them yet) shouldn't be that hard and I'd be happy to do
> that. Perhaps with the exception of union types, but if you think it's
> important, I can propose something for them, too.
>
>> I feel a necessary early step is to actually define our schema language,
>> and separate syntactic sugar from its core.  Introspection wants the
>> core, but for schema development, a modest amount of sugar is
>> convenient.  Some of the current schema syntax needs to be redefined as
>> sugar for a suitable desugared form.  We've discussed this for "asterisk
>> means optional" already.  Eric's series brings us closer, it just needs
>> to be finished.
>
> Our schema language is an implementation detail, we shouldn't worry
> about it too much in this discussion. As long as we agree that arguments
> do have a name, a type and a boolean that describes whether they are
> optional (and I don't think anyone would deny that), we can use that in
> a future-proof external API.
>
>> If we can't crack the whole problem in the next development cycle,
>> perhaps we can identify a minimally useful sub-schema that is unlikely
>> to be affected by the above work.  We could then expose that early, with
>> the idea of extending it later.
>
> We've done that for several years and we haven't cracked it. We need to
> start doing something. Now.
>
> Look at blockdev. We only started moving because we stopped discussing
> and actually started hacking on it. And blockdev is much more complex
> and we still haven't figured out all the details. In comparison with
> blockdev, schema introspection doesn't look that hard any more.

I don't think you can compare blockdev-add and introspection at this
time.

The history of our introspection efforts isn't "we've done that for
several years" without success.  What we've done is dicking around with
various half-hearted work-arounds.  The only real attempt at the problem
so far has been Amos's, and we let his effort stall.  I've been able to
spare enough time for introspection to shoot holes into proposals, but
not for real analysis.

The history of our blockdev effort isn't "we only started moving because
we stopped discussing and actually started hacking on it".  I had been
hacking on it on and off (mostly off) for a long time, but produced only
solutions to preliminary problems (and a few unfinished, hacked-to-death
branches in my private repo).  Useful preliminaries alright, but not
enough.  We "started moving" when you were able to invest a really big
chunk of time, and approached the problem from a different angle.

I readily accept the idea that some problems defy analysis without
experiments (read: actual coding).  With a bit of luck, the code can
even grow into a solution.  But that's no reason to dismiss analysis
without even giving it a try.

Let's give it a try.


[1] It's whatever scripts/qapi*.py accept, except when their output is
nonsense.  A product of "doing something, now" incremental development.
Eric's stalled series improves the situation.  If you want to peek:
http://repo.or.cz/w/qemu/armbru.git/shortlog/refs/heads/review-eblake-qapi

Re: [Qemu-devel] [PATCH 0/1] Get the list of arguments from a QMP command

[Alberto Garcia]

On Sat, Mar 14, 2015 at 05:12:33PM +0100, Markus Armbruster wrote:

  [QMP introspection]
> Detail.  I want us to figure out what information we want to provide
> first, how to encode it second, and how to fit it into the existing
> introspection interfaces third.

Ok, I managed to find some time to take look at this.

Here's a rough proposal of the kind of information we can provide
and how to encode it. The type names I'm using are probably not very
well thought-out, but my intention at this point is to discuss the
structure rather than the names.

So let's start with the basics:

  { 'type': 'SimpleTypeInfo',
    'data': {
        'name': 'str',
        'array': 'bool'
    }
  }

This would be what describes a single, named data type (as opposed
to a dictionary). The name would refer to any of the build-in types,
complex types, enums or unions. Alternatively instead of an 'str' it
could also be an enum, which could be auto-generated and would contain
all possible type names. I don't know if this last option is feasible,
I had the impression that it's not for commands with 'gen' == 'no',
but correct me if I'm wrong.

Then I think that all instances where a simple type can appear also
allow arrays, so an additional boolean field can be used for that,
e.g.:

    'returns': 'str'           // name: str, array: no
    'returns': ['EventInfo']   // name: EventInfo, array: yes

Next:

  { 'type': 'DataFieldInfo',
    'data': {
        'name': 'str',
        'optional': 'bool',
        'type': 'SimpleTypeInfo'
    }
  }

That's for command arguments and fields in complex data types and
events. I think this one does not need much explanation. I admit that
the name that I chose is not very good :)

  { 'type': 'ComplexTypeInfo',
    'data': {
        'name': 'str',
        'fields': [ 'DataFieldInfo' ]
    }
  }

  { 'type': 'EnumInfo',
    'data': {
        'name': 'str',
        'values': [ 'str' ]
    }
  }

  { 'type': 'EventInfo',
    'data': {
        'name': 'str',
        'fields': [ 'DataFieldInfo' ]
    }
  }

This is for complex types, unions and events. I also think they don't
need much explanation. ComplexTypeInfo and EventInfo could be merged
into the same type, although I guess it's a good idea to keep them
separated in case one of them needs to be extended in the future.

  { 'type': 'UnionFieldInfo',
    'data': {
        'name': 'str',
        'type': 'str',
    }
  }

  { 'type': 'UnionInfo',
    'data': {
        'name': 'str',
        '*base': 'str'
        '*discriminator': 'str',
        'fields': [ 'UnionFieldInfo' ]
    }
  }

Unions. The type for 'base' could be (instead of 'str') an
autogenerated enum with all complex data types. 'discriminator' would
be 'type' for the default case. If the union is anonymous then this
field would be absent.

  { 'type': 'CommandInfo',
    'data': {
        'name': 'str',
        'args': [ 'DataFieldInfo' ],
        '*returns': 'SimpleTypeInfo',
        'gen': 'bool',
        'success-response': 'bool'
    }
  }

Commands. As far as I can see all of them return a single type (or an
array thereof), but this could be replaced if we want something else.

I think that's all.

Then as you said we could hide complex types and flatten them all when
we expose them. It would probably makes things easier in terms of
allowing changes in the API, but sounds like it would make the life of
API users more difficult? I don't know.

Last, about the query commands, I don't think they're so important
at the moment. Whether we want to have 'query-schema' or a set of
'query-*' for different things, or a combination of both, is something
that does not really have much impact on this discussion. Once we know
what we want to expose, the commands will come naturally I think.

Berto

Re: [Qemu-devel] [PATCH 0/1] Get the list of arguments from a QMP command

[Markus Armbruster]

I'm up to the ears in a query-schema prototype.  I'll reply to this as
soon as I can.

Re: [Qemu-devel] [PATCH 0/1] Get the list of arguments from a QMP command

[Kevin Wolf]

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

Am 06.03.2015 um 18:11 hat Eric Blake geschrieben:
> On 02/24/2015 06:51 AM, Alberto Garcia wrote:
> > Hello,
> > 
> > this is a follow-up to the comments from Eric Blake about my patches
> > to extend the block streaming API:
> > 
> >    https://lists.gnu.org/archive/html/qemu-devel/2015-02/msg04231.html
> > 
> > Since QEMU doesn't have an easy way to tell the arguments that a
> > particular QMP command supports, and it seems that it would be useful
> > for libvirt and possibly others, I decided to write a simple patch
> > that adds that feature.
> 
> This is potentially a step in the right direction for full
> introspection, but I'm a bit worried that it's half-baked.  That is, it
> only states whether an argument is present or not, but doesn't say what
> type those arguments are, and most glaringly, doesn't tell me anything
> about type changes, such as adding new enum values or new struct
> members.  In other words, while this interface might help libvirt, it
> won't solve all the qapi questions that libvirt has, and I'm worried
> that committing this and then adding full introspection will burden us
> with multiple implementations to keep running.

I suggested going this route to Alberto because our previous attempts on
doing the full introspection all have failed, and requesting to do such
a large project on the side just for getting in some block layer
improvements wouldn't be fair.

Sometimes it's just more realistic to approach things incrementally.
It's the same thing with the blockdev work: We're still not fully there,
but discussing about the full solution for three more years wouldn't
have helped either.

As far as I can tell, this approach is extensible enough that the other
relevant information can be added later without breaking the API. I
believe this is what we should concentrate on now rather than insisting
that the implementation must be complete from the start.

> > Feedback is welcome.
> 
> I'm still thinking about the actual patch, and whether we want to commit
> to this or just bite the bullet and go for full introspection.  At any
> rate, it's a bit late for 2.3, so we have the full 2.4 cycle to get it
> right.

If someone were to provide full introspection for 2.4, that would be
best. But I'd rather merge something like this than having nothing for
another couple of releases.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]