Patchwork [14/22] qapi: add query-version QMP command

login
register
mail settings
Submitter Anthony Liguori
Date March 7, 2011, 1:22 a.m.
Message ID <1299460984-15849-15-git-send-email-aliguori@us.ibm.com>
Download mbox | patch
Permalink /patch/85620/
State New
Headers show

Comments

Anthony Liguori - March 7, 2011, 1:22 a.m.
This is used internally by QMP.  It's also a pretty good example of a typical
command conversion.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Stefan Hajnoczi - March 7, 2011, 1:35 p.m.
On Mon, Mar 7, 2011 at 1:22 AM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> diff --git a/qmp-schema.json b/qmp-schema.json
> index e69de29..b343f5e 100644
> --- a/qmp-schema.json
> +++ b/qmp-schema.json
> @@ -0,0 +1,38 @@
> +# *-*- Mode: Python -*-*

By the way JSON does not seem to support comments (neither /* */ nor
#).  So this comment feature you're using is an extension to JSON.  I
also can't see how to do multi-line strings in JSON so "doc-strings"
aren't in the cards either.

Stefan
Anthony Liguori - March 7, 2011, 1:41 p.m.
On 03/07/2011 07:35 AM, Stefan Hajnoczi wrote:
> On Mon, Mar 7, 2011 at 1:22 AM, Anthony Liguori<aliguori@us.ibm.com>  wrote:
>    
>> diff --git a/qmp-schema.json b/qmp-schema.json
>> index e69de29..b343f5e 100644
>> --- a/qmp-schema.json
>> +++ b/qmp-schema.json
>> @@ -0,0 +1,38 @@
>> +# *-*- Mode: Python -*-*
>>      
> By the way JSON does not seem to support comments (neither /* */ nor
> #).  So this comment feature you're using is an extension to JSON.

Yeah, it's only loosely JSON as I don't use a JSON parser.

Regards,

Anthony Liguori

>    I
> also can't see how to do multi-line strings in JSON so "doc-strings"
> aren't in the cards either.
>
> Stefan
>
>
Avi Kivity - March 9, 2011, 1:28 p.m.
On 03/07/2011 03:41 PM, Anthony Liguori wrote:
> On 03/07/2011 07:35 AM, Stefan Hajnoczi wrote:
>> On Mon, Mar 7, 2011 at 1:22 AM, Anthony Liguori<aliguori@us.ibm.com>  
>> wrote:
>>> diff --git a/qmp-schema.json b/qmp-schema.json
>>> index e69de29..b343f5e 100644
>>> --- a/qmp-schema.json
>>> +++ b/qmp-schema.json
>>> @@ -0,0 +1,38 @@
>>> +# *-*- Mode: Python -*-*
>> By the way JSON does not seem to support comments (neither /* */ nor
>> #).  So this comment feature you're using is an extension to JSON.
>
> Yeah, it's only loosely JSON as I don't use a JSON parser.

Goes kind of against all the buzzwords you're letting fly here...
Avi Kivity - March 9, 2011, 1:36 p.m.
On 03/07/2011 03:22 AM, Anthony Liguori wrote:
> This is used internally by QMP.  It's also a pretty good example of a typical
> command conversion.
>
> +##
> +{ 'VersionInfo': {'qemu': {'major': 'int', 'minor': 'int', 'micro': 'int'},
> +                  'package': 'str'} }
> +
> +##
> +# @query-version:
> +#
> +# Returns the current version of QEMU.
> +#
> +# Returns:  A @VersionInfo object describing the current version of QEMU.
> +#
> +# Since: 0.14.0
> +##
> +[ 'query-version', {}, {}, 'VersionInfo' ]

(just picking on a patch that has a bit of schema in it)

Something that can be added to the schema are default values for newly 
added parameters.  This way we can avoid an explosion of commands where 
adding an optional parameter suffices; should be easier for the user to 
pick the right command and easier for us to document and support.
Anthony Liguori - March 9, 2011, 1:44 p.m.
On 03/09/2011 07:28 AM, Avi Kivity wrote:
> On 03/07/2011 03:41 PM, Anthony Liguori wrote:
>> On 03/07/2011 07:35 AM, Stefan Hajnoczi wrote:
>>> On Mon, Mar 7, 2011 at 1:22 AM, Anthony 
>>> Liguori<aliguori@us.ibm.com>  wrote:
>>>> diff --git a/qmp-schema.json b/qmp-schema.json
>>>> index e69de29..b343f5e 100644
>>>> --- a/qmp-schema.json
>>>> +++ b/qmp-schema.json
>>>> @@ -0,0 +1,38 @@
>>>> +# *-*- Mode: Python -*-*
>>> By the way JSON does not seem to support comments (neither /* */ nor
>>> #).  So this comment feature you're using is an extension to JSON.
>>
>> Yeah, it's only loosely JSON as I don't use a JSON parser.
>
> Goes kind of against all the buzzwords you're letting fly here...

The schema defines arguments in a dictionary because in QMP, the 
argument order doesn't matter.  But the argument order matters in C so I 
need to use a custom parser to preserve dictionary order.

There's no way to do commenting in JSON and I really wanted to have 
inline documentation.

But otherwise, it's valid JSON.

Regards,

Anthony Liguori
Avi Kivity - March 9, 2011, 1:51 p.m.
On 03/09/2011 03:44 PM, Anthony Liguori wrote:
>>> Yeah, it's only loosely JSON as I don't use a JSON parser.
>>
>> Goes kind of against all the buzzwords you're letting fly here...
>
>
> The schema defines arguments in a dictionary because in QMP, the 
> argument order doesn't matter.  But the argument order matters in C so 
> I need to use a custom parser to preserve dictionary order.

We could extend our parser to annotate the dictionary with the original 
order.  Not worth it though.

>
> There's no way to do commenting in JSON and I really wanted to have 
> inline documentation.
>
> But otherwise, it's valid JSON.
>

We should then have a transformation that generates a valid json for 
clients to use.  We could even include the documentation as a 'doc': key.
Anthony Liguori - March 9, 2011, 2:11 p.m.
On 03/09/2011 07:36 AM, Avi Kivity wrote:
> On 03/07/2011 03:22 AM, Anthony Liguori wrote:
>> This is used internally by QMP.  It's also a pretty good example of a 
>> typical
>> command conversion.
>>
>> +##
>> +{ 'VersionInfo': {'qemu': {'major': 'int', 'minor': 'int', 'micro': 
>> 'int'},
>> +                  'package': 'str'} }
>> +
>> +##
>> +# @query-version:
>> +#
>> +# Returns the current version of QEMU.
>> +#
>> +# Returns:  A @VersionInfo object describing the current version of 
>> QEMU.
>> +#
>> +# Since: 0.14.0
>> +##
>> +[ 'query-version', {}, {}, 'VersionInfo' ]
>
> (just picking on a patch that has a bit of schema in it)

If you want to see more of the schema in action 
http://repo.or.cz/w/qemu/aliguori.git/blob/refs/heads/glib:/qmp-schema.json

> Something that can be added to the schema are default values for newly 
> added parameters.  This way we can avoid an explosion of commands 
> where adding an optional parameter suffices; should be easier for the 
> user to pick the right command and easier for us to document and support.

Adding a parameter to a command, even if the schema specifies a default 
value, is going to break the C library ABI so it's something we should 
strongly discourage.

We definitely want to systematically document defaults but the question 
is whether that belongs in the documentation for the command or the 
schema itself.  Since a default doesn't affect the wire protocol in any 
way, shape, or form, I think there a pretty strong argument that it 
belongs in the documentation and not the schema.

gtk-doc typically uses a tag to identify defaults.  I think it's 
something like #default although that is purely a marking concept (the 
value isn't parsed out or anything).  Whether we'd want to automatically 
parse the value following the #default tag and change the internal 
signature is probably a discussion we can defer.  Since this only really 
should apply to structures, I suspect it's not a huge win.

Regards,

Anthony Liguori
Anthony Liguori - March 9, 2011, 2:13 p.m.
On 03/09/2011 07:51 AM, Avi Kivity wrote:
> On 03/09/2011 03:44 PM, Anthony Liguori wrote:
>>>> Yeah, it's only loosely JSON as I don't use a JSON parser.
>>>
>>> Goes kind of against all the buzzwords you're letting fly here...
>>
>>
>> The schema defines arguments in a dictionary because in QMP, the 
>> argument order doesn't matter.  But the argument order matters in C 
>> so I need to use a custom parser to preserve dictionary order.
>
> We could extend our parser to annotate the dictionary with the 
> original order.  Not worth it though.
>
>>
>> There's no way to do commenting in JSON and I really wanted to have 
>> inline documentation.
>>
>> But otherwise, it's valid JSON.
>>
>
> We should then have a transformation that generates a valid json for 
> clients to use.  We could even include the documentation as a 'doc': key.

Yes, as I mentioned on the call, that's my plan for 0.15.  I hadn't 
thought about the doc bit though.

Regards,

Anthony Liguori
Avi Kivity - March 9, 2011, 2:37 p.m.
On 03/09/2011 04:11 PM, Anthony Liguori wrote:
>> (just picking on a patch that has a bit of schema in it)
>
>
> If you want to see more of the schema in action 
> http://repo.or.cz/w/qemu/aliguori.git/blob/refs/heads/glib:/qmp-schema.json

Thanks.  Something a little worrying is the reliance on capitalization 
and punctuation ( {} vs [] ) do distinguish among the different types of 
declarations.  It's not immediately clear if something is a type, event, 
or command.

We could do

[

{ 'type': 'MyType', fields: [['a', 'str'], ['b', 'int'], ['c', 
'AnotherType']] }
{ 'event': 'MY_EVENT', 'arguments': [ ... ] }
{ 'command': 'my-command', 'arguments': [ ... ], 'return': ... }

]

which leaves us room for additional metainformation.

The concern is more about non-qemu consumers of the schema.

>
>> Something that can be added to the schema are default values for 
>> newly added parameters.  This way we can avoid an explosion of 
>> commands where adding an optional parameter suffices; should be 
>> easier for the user to pick the right command and easier for us to 
>> document and support.
>
> Adding a parameter to a command, even if the schema specifies a 
> default value, is going to break the C library ABI so it's something 
> we should strongly discourage.

We could add compatibility signatures when we extend a command:


{ 'command': 'x', arguments: [['a', 'str']], return: ...,
    'signatures': { 'x': [], 'x2': ['a'] } }

That lets the wire protocol extend x without introducing a new command, 
but for libqmp it adds a new x2() API with the new parameter.

>
> We definitely want to systematically document defaults but the 
> question is whether that belongs in the documentation for the command 
> or the schema itself.  Since a default doesn't affect the wire 
> protocol in any way, shape, or form, I think there a pretty strong 
> argument that it belongs in the documentation and not the schema.

Agree.

>
> gtk-doc typically uses a tag to identify defaults.  I think it's 
> something like #default although that is purely a marking concept (the 
> value isn't parsed out or anything).  Whether we'd want to 
> automatically parse the value following the #default tag and change 
> the internal signature is probably a discussion we can defer.  Since 
> this only really should apply to structures, I suspect it's not a huge 
> win.
Anthony Liguori - March 9, 2011, 2:47 p.m.
On 03/09/2011 08:37 AM, Avi Kivity wrote:
> On 03/09/2011 04:11 PM, Anthony Liguori wrote:
>>> (just picking on a patch that has a bit of schema in it)
>>
>>
>> If you want to see more of the schema in action 
>> http://repo.or.cz/w/qemu/aliguori.git/blob/refs/heads/glib:/qmp-schema.json
>
> Thanks.  Something a little worrying is the reliance on capitalization 
> and punctuation ( {} vs [] ) do distinguish among the different types 
> of declarations.  It's not immediately clear if something is a type, 
> event, or command.
>
> We could do
>
> [
>
> { 'type': 'MyType', fields: [['a', 'str'], ['b', 'int'], ['c', 
> 'AnotherType']] }
> { 'event': 'MY_EVENT', 'arguments': [ ... ] }
> { 'command': 'my-command', 'arguments': [ ... ], 'return': ... }
>
> ]
>
> which leaves us room for additional metainformation.
>
> The concern is more about non-qemu consumers of the schema.

Yeah, I dislike it too and I've been leaning towards changing it.

My preference would be:

{ 'type': 'MyType', 'fields': { 'a': 'str', 'b': 'int', 'c': 
'AnotherType' } }
{ 'event': 'MY_EVENT', 'arguments': {...} }
{ 'command': 'my-command', 'arguments': {...}, 'returns': 'int' }

I do prefer the dictionary syntax for arguments over a list because a 
list implies order.  Plus I think the syntax is just awkward and a whole 
lot easier to get wrong (too many/few elements in list).

I don't think I want to make this sort of change just yet.  Also note 
that the schema that will be exposed over the wire is not directly 
related to the schema we use for code generation.

>>> Something that can be added to the schema are default values for 
>>> newly added parameters.  This way we can avoid an explosion of 
>>> commands where adding an optional parameter suffices; should be 
>>> easier for the user to pick the right command and easier for us to 
>>> document and support.
>>
>> Adding a parameter to a command, even if the schema specifies a 
>> default value, is going to break the C library ABI so it's something 
>> we should strongly discourage.
>
> We could add compatibility signatures when we extend a command:
>
>
> { 'command': 'x', arguments: [['a', 'str']], return: ...,
>    'signatures': { 'x': [], 'x2': ['a'] } }
>
> That lets the wire protocol extend x without introducing a new 
> command, but for libqmp it adds a new x2() API with the new parameter.

I'd rather just not add arguments.

Treating QMP as a C API makes it easier for us to maintain compatibility 
both internally and externally.

Regards,

Anthony Liguori
Avi Kivity - March 10, 2011, 12:41 p.m.
On 03/09/2011 04:47 PM, Anthony Liguori wrote:
>> [
>>
>> { 'type': 'MyType', fields: [['a', 'str'], ['b', 'int'], ['c', 
>> 'AnotherType']] }
>> { 'event': 'MY_EVENT', 'arguments': [ ... ] }
>> { 'command': 'my-command', 'arguments': [ ... ], 'return': ... }
>>
>> ]
>>
>> which leaves us room for additional metainformation.
>>
>> The concern is more about non-qemu consumers of the schema.
>
>
> Yeah, I dislike it too and I've been leaning towards changing it.
>
> My preference would be:
>
> { 'type': 'MyType', 'fields': { 'a': 'str', 'b': 'int', 'c': 
> 'AnotherType' } }
> { 'event': 'MY_EVENT', 'arguments': {...} }
> { 'command': 'my-command', 'arguments': {...}, 'returns': 'int' }
>
> I do prefer the dictionary syntax for arguments over a list because a 
> list implies order.  Plus I think the syntax is just awkward and a 
> whole lot easier to get wrong (too many/few elements in list).

Yeah.  We can rationalize it by saying that most dynamic consumers of 
the schema will not care about argument order, and that if they do, they 
can implement a custom parser.

> I don't think I want to make this sort of change just yet.  Also note 
> that the schema that will be exposed over the wire is not directly 
> related to the schema we use for code generation.

Right, we have to nail down the format for the former, though.
Avi Kivity - March 10, 2011, 12:46 p.m.
On 03/10/2011 02:41 PM, Avi Kivity wrote:
>> I don't think I want to make this sort of change just yet.  Also note 
>> that the schema that will be exposed over the wire is not directly 
>> related to the schema we use for code generation.
>
> Right, we have to nail down the format for the former, though.
>

btw.  Back in the day when json was proposed vs. a custom text-line 
oriented protocol, it beat established RPCs because they were all so 
cumbersome with IDLs and code generation and general heavyweightness.  
And now we're making our json-rpc exactly like that.  There's a moral in 
there somewhere.
Anthony Liguori - March 10, 2011, 1:52 p.m.
On 03/10/2011 06:41 AM, Avi Kivity wrote:
>> My preference would be:
>>
>> { 'type': 'MyType', 'fields': { 'a': 'str', 'b': 'int', 'c': 
>> 'AnotherType' } }
>> { 'event': 'MY_EVENT', 'arguments': {...} }
>> { 'command': 'my-command', 'arguments': {...}, 'returns': 'int' }
>>
>> I do prefer the dictionary syntax for arguments over a list because a 
>> list implies order.  Plus I think the syntax is just awkward and a 
>> whole lot easier to get wrong (too many/few elements in list).
>
>
> Yeah.  We can rationalize it by saying that most dynamic consumers of 
> the schema will not care about argument order, and that if they do, 
> they can implement a custom parser.
>
>> I don't think I want to make this sort of change just yet.  Also note 
>> that the schema that will be exposed over the wire is not directly 
>> related to the schema we use for code generation.
>
> Right, we have to nail down the format for the former, though.

There's also enum syntax which I guess would be:

{ 'enum': 'MyEnum', values: [ 'value1', 'value2', 'value3' ] }

The other thing that I'm delaying until post 0.15 is errors.  I'd like 
to define errors as part of the schema.  Something like:

{ 'error': 'FileNotFound', 'arguments': { 'filename': 'str' } }

Regards,

Anthony Liguori

Patch

diff --git a/Makefile.objs b/Makefile.objs
index 5dae800..e1a2756 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -103,7 +103,7 @@  common-obj-y += block-migration.o
 common-obj-y += pflib.o
 common-obj-y += bitmap.o bitops.o
 common-obj-y += qmp-marshal-types.o qmp-marshal-types-core.o
-common-obj-y += qmp-core.o qmp-marshal.o
+common-obj-y += qmp-core.o qmp-marshal.o qmp.o
 
 common-obj-$(CONFIG_BRLAPI) += baum.o
 common-obj-$(CONFIG_POSIX) += migration-exec.o migration-unix.o migration-fd.o
diff --git a/qmp-schema.json b/qmp-schema.json
index e69de29..b343f5e 100644
--- a/qmp-schema.json
+++ b/qmp-schema.json
@@ -0,0 +1,38 @@ 
+# *-*- Mode: Python -*-*
+### 0.14.0 commands.  Do not modify. ###
+
+##
+# @VersionInfo:
+#
+# A description of QEMU's version.
+#
+# @qemu.major:  The major version of QEMU
+#
+# @qemu.minor:  The minor version of QEMU
+#
+# @qemu.micro:  The micro version of QEMU.  By current convention, a micro
+#               version of 50 signifies a development branch.  A micro version
+#               greater than or equal to 90 signifies a release candidate for
+#               the next minor version.  A micro version of less than 50
+#               signifies a stable release.
+#
+# @package:     QEMU will always set this field to an empty string.  Downstream
+#               versions of QEMU should set this to a non-empty string.  The
+#               exact format depends on the downstream however it highly
+#               recommended that a unique name is used.
+#
+# Since: 0.14.0
+##
+{ 'VersionInfo': {'qemu': {'major': 'int', 'minor': 'int', 'micro': 'int'},
+                  'package': 'str'} }
+
+##
+# @query-version:
+#
+# Returns the current version of QEMU.
+#
+# Returns:  A @VersionInfo object describing the current version of QEMU.
+#
+# Since: 0.14.0
+##
+[ 'query-version', {}, {}, 'VersionInfo' ]
diff --git a/qmp.c b/qmp.c
new file mode 100644
index 0000000..7b626f5
--- /dev/null
+++ b/qmp.c
@@ -0,0 +1,31 @@ 
+/*
+ * QAPI
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.  See
+ * the COPYING.LIB file in the top-level directory.
+ */
+#include "qemu-common.h"
+#include "qmp-core.h"
+#include "qmp.h"
+
+VersionInfo *qmp_query_version(Error **err)
+{
+    VersionInfo *info = qmp_alloc_version_info();
+    const char *version = QEMU_VERSION;
+    char *tmp;
+
+    info->qemu.major = strtol(version, &tmp, 10);
+    tmp++;
+    info->qemu.minor = strtol(tmp, &tmp, 10);
+    tmp++;
+    info->qemu.micro = strtol(tmp, &tmp, 10);
+    info->package = qemu_strdup(QEMU_PKGVERSION);
+
+    return info;
+}
+