Patchwork [2/2] monitor: introduce query-config-schema command

login
register
mail settings
Submitter Amos Kong
Date April 24, 2013, 12:47 p.m.
Message ID <1366807646-8473-2-git-send-email-akong@redhat.com>
Download mbox | patch
Permalink /patch/239197/
State New
Headers show

Comments

Amos Kong - April 24, 2013, 12:47 p.m.
Libvirt has no way to probe if an option or property is supported,
This patch introdues a new qmp command to query configuration schema
information. hmp command isn't added because it's not needed.

Signed-off-by: Amos Kong <akong@redhat.com>
CC: Osier Yang <jyang@redhat.com>
CC: Anthony Liguori <aliguori@us.ibm.com>
---
 qapi-schema.json   |   29 +++++++++++++++++++++++++++++
 qmp-commands.hx    |   40 ++++++++++++++++++++++++++++++++++++++++
 util/qemu-config.c |   40 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 109 insertions(+), 0 deletions(-)
Eric Blake - April 24, 2013, 2:38 p.m.
On 04/24/2013 06:47 AM, Amos Kong wrote:
> Libvirt has no way to probe if an option or property is supported,
> This patch introdues a new qmp command to query configuration schema
> information. hmp command isn't added because it's not needed.

Agreed that no HMP counterpart is needed.  However, I don't think we
have quite the right interface yet.

> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> CC: Osier Yang <jyang@redhat.com>
> CC: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  qapi-schema.json   |   29 +++++++++++++++++++++++++++++
>  qmp-commands.hx    |   40 ++++++++++++++++++++++++++++++++++++++++
>  util/qemu-config.c |   40 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 109 insertions(+), 0 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 751d3c2..aeab057 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3505,3 +3505,32 @@
>      '*asl_compiler_rev':  'uint32',
>      '*file':              'str',
>      '*data':              'str' }}
> +
> +##
> +# @ConfigSchemaInfo:
> +#
> +# Configration schema information.
> +#
> +# @option: option name
> +#
> +# @params: parameters strList of one option

Why just a strList?  That only tells me option names.  But we already
know so much more than that - we know the type and the help string.

> +#
> +# Since 1.5
> +##
> +{ 'type': 'ConfigSchemaInfo', 'data': {'option': 'str', 'params': ['str']} }

I'd rather see an array of structs, more closely mirroring what
include/qemu/option.h gives us:

# JSON representation of values of QEMUOptionParType, may grow in future
{ 'enum': 'ConfigParamType',
  'data': [ 'flag', 'number', 'size', 'string' ] }

# JSON representation of QEMUOptionParameter, may grow in future
# @help is optional if no text was present
{ 'type': 'ConfigParamInfo',
  'data': { 'name': 'str', 'type': 'ConfigParamType', '*help':'str' } }

# Each command line option, and its list of parameters
{ 'type': 'ConfigSchemaInfo',
  'data': { 'option':'str', 'params': ['ConfigParamInfo'] } }

And that means we no longer have ['str'], which bypasses the need for
your patch 1/2.

> +
> +##
> +# @query-config-schema
> +#
> +# Query configuration schema information of options
> +#
> +# @option: #optional option name
> +#
> +# Returns: returns @ConfigSchemaInfo if option is assigned, returns
> +#          @ConfigSchemaInfo list if no option is assigned, returns an error
> +#          QERR_INVALID_OPTION_GROUP if assigned option doesn't exist.

That didn't read well.  Also, QERR_INVALID_OPTION_GROUP is a generic
error; we don't mention any other QERR_* names in qapi-schema.json.  How
about:

Returns: list of @ConfigSchemaInfo for all options (or for the given
         @option).  Returns an error if a given @option doesn't exist.

> +#
> +# Since 1.5
> +##
> +{'command': 'query-config-schema', 'data': {'*option': 'str'},
> + 'returns': ['ConfigSchemaInfo']}

This part looks good.

> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 4d65422..c6399be 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2414,6 +2414,46 @@ EQMP
>          .args_type  = "",
>          .mhandler.cmd_new = qmp_marshal_input_query_uuid,
>      },
> +SQMP
> +query-config-schema
> +------------
> +
> +Show configuration schema.
> +
> +Return configuration schema of one option if option is assigned, return
> +configuration schema list of all options if no option is assigned. return
> +an error QERR_INVALID_OPTION_GROUP if assigned option doesn't exist.

Again, QERR_INVALID_OPTION_GROUP is not a defined error (it is shorthand
for a specific message for a GenericError class).

> +
> +- "option": option name
> +- "params": parameters string list of one option
> +
> +Example:
> +
> +-> {"execute": "query-config-schema", "arguments" : {"option": "boot-opts"}}
> +<- {
> +    "return": [
> +        {
> +            "params": [
> +                "strict",
> +                "reboot-timeout",
> +                "splash-time",
> +                "splash",
> +                "menu",
> +                "once",
> +                "order"
> +            ],
> +            "option": "boot-opts"
> +        }
> +    ]
> +  }

Back to my desire for more structure, I'd like to see:


-> {"execute": "query-config-schema", "arguments" : {"option": "boot-opts"}}
<- {
    "return": [
        {
            "params": [
                {
                    "name": "strict",
                    "type": "string"
                },
                {
                    "name": "reboot-timeout",
                    "type": "string"
                },
                ...
            ],
            "option": "boot-opts"
        }
     ]
  }

Another more interesting example, showing why the "type" and optional
"help" fields are useful, assuming
https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg04810.html goes in:

-> {"execute": "query-config-schema", "arguments" : {"option": "drive"}}
<- {
    "return": [
        {
            "params": [
                {
                    "name": "bus",
                    "type": "number",
                    "help": "bus number"
                },
                {
                    "name": "if",
                    "type": "string",
                    "help": "interface (ide, scsi, sd, mtd, floppy,
pflash, virtio)"
                },
                ...
            ],
            "option": "drive"
        }
     ]
  }


>  
> +ConfigSchemaInfoList *qmp_query_config_schema(bool has_option,
> +                                              const char *option, Error **errp)
> +{
> +    ConfigSchemaInfoList *conf_list = NULL, *entry;
> +    ConfigSchemaInfo *info;
> +    strList *str_list = NULL, *str_entry;
> +    int entries, i, j;
> +
> +    entries = ARRAY_SIZE(vm_config_groups);
> +
> +    for (i = 0; i < entries; i++) {
> +        if (vm_config_groups[i] != NULL &&
> +            (!has_option || !strcmp(option, vm_config_groups[i]->name))) {
> +            info = g_malloc0(sizeof(*info));

This part of the iteration looks fine.

> +            info->option = g_strdup(vm_config_groups[i]->name);
> +            str_list = NULL;
> +
> +            for (j = 0; vm_config_groups[i]->desc[j].name != NULL; j++) {
> +                str_entry = g_malloc0(sizeof(*str_entry));
> +                str_entry->value = g_strdup(vm_config_groups[i]->desc[j].name);
> +                str_entry->next = str_list;
> +                str_list = str_entry;
> +            }

Here, you'd need to allocate a bit more structure, to match the JSON I
proposed above.

> +
> +            info->params = str_list;
> +            entry = g_malloc0(sizeof(*entry));
> +            entry->value = info;
> +            entry->next = conf_list;
> +            conf_list = entry;
> +        }
> +    }
> +
> +    if (conf_list == NULL) {
> +        error_set(errp, QERR_INVALID_OPTION_GROUP, option);
> +    }
> +
> +    return conf_list;
> +}
> +
>  QemuOptsList *qemu_find_opts_err(const char *group, Error **errp)
>  {
>      return find_list(vm_config_groups, group, errp);
> 

Looking forward to v2.
Anthony Liguori - April 24, 2013, 4:45 p.m.
Eric Blake <eblake@redhat.com> writes:

> On 04/24/2013 06:47 AM, Amos Kong wrote:
>> Libvirt has no way to probe if an option or property is supported,
>> This patch introdues a new qmp command to query configuration schema
>> information. hmp command isn't added because it's not needed.
>
> Agreed that no HMP counterpart is needed.  However, I don't think we
> have quite the right interface yet.
>
>> 
>> Signed-off-by: Amos Kong <akong@redhat.com>
>> CC: Osier Yang <jyang@redhat.com>
>> CC: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>>  qapi-schema.json   |   29 +++++++++++++++++++++++++++++
>>  qmp-commands.hx    |   40 ++++++++++++++++++++++++++++++++++++++++
>>  util/qemu-config.c |   40 ++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 109 insertions(+), 0 deletions(-)
>> 
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 751d3c2..aeab057 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -3505,3 +3505,32 @@
>>      '*asl_compiler_rev':  'uint32',
>>      '*file':              'str',
>>      '*data':              'str' }}
>> +
>> +##
>> +# @ConfigSchemaInfo:
>> +#
>> +# Configration schema information.
>> +#
>> +# @option: option name
>> +#
>> +# @params: parameters strList of one option
>
> Why just a strList?  That only tells me option names.  But we already
> know so much more than that - we know the type and the help string.
>
>> +#
>> +# Since 1.5
>> +##
>> +{ 'type': 'ConfigSchemaInfo', 'data': {'option': 'str', 'params': ['str']} }
>
> I'd rather see an array of structs, more closely mirroring what
> include/qemu/option.h gives us:
>
> # JSON representation of values of QEMUOptionParType, may grow in future
> { 'enum': 'ConfigParamType',
>   'data': [ 'flag', 'number', 'size', 'string' ] }
>
> # JSON representation of QEMUOptionParameter, may grow in future
> # @help is optional if no text was present
> { 'type': 'ConfigParamInfo',
>   'data': { 'name': 'str', 'type': 'ConfigParamType', '*help':'str' } }
>
> # Each command line option, and its list of parameters
> { 'type': 'ConfigSchemaInfo',
>   'data': { 'option':'str', 'params': ['ConfigParamInfo'] } }
>
> And that means we no longer have ['str'], which bypasses the need for
> your patch 1/2.

Strong Ack on this schema.

Regards,

Anthony Liguori

Patch

diff --git a/qapi-schema.json b/qapi-schema.json
index 751d3c2..aeab057 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3505,3 +3505,32 @@ 
     '*asl_compiler_rev':  'uint32',
     '*file':              'str',
     '*data':              'str' }}
+
+##
+# @ConfigSchemaInfo:
+#
+# Configration schema information.
+#
+# @option: option name
+#
+# @params: parameters strList of one option
+#
+# Since 1.5
+##
+{ 'type': 'ConfigSchemaInfo', 'data': {'option': 'str', 'params': ['str']} }
+
+##
+# @query-config-schema
+#
+# Query configuration schema information of options
+#
+# @option: #optional option name
+#
+# Returns: returns @ConfigSchemaInfo if option is assigned, returns
+#          @ConfigSchemaInfo list if no option is assigned, returns an error
+#          QERR_INVALID_OPTION_GROUP if assigned option doesn't exist.
+#
+# Since 1.5
+##
+{'command': 'query-config-schema', 'data': {'*option': 'str'},
+ 'returns': ['ConfigSchemaInfo']}
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 4d65422..c6399be 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2414,6 +2414,46 @@  EQMP
         .args_type  = "",
         .mhandler.cmd_new = qmp_marshal_input_query_uuid,
     },
+SQMP
+query-config-schema
+------------
+
+Show configuration schema.
+
+Return configuration schema of one option if option is assigned, return
+configuration schema list of all options if no option is assigned. return
+an error QERR_INVALID_OPTION_GROUP if assigned option doesn't exist.
+
+- "option": option name
+- "params": parameters string list of one option
+
+Example:
+
+-> {"execute": "query-config-schema", "arguments" : {"option": "boot-opts"}}
+<- {
+    "return": [
+        {
+            "params": [
+                "strict",
+                "reboot-timeout",
+                "splash-time",
+                "splash",
+                "menu",
+                "once",
+                "order"
+            ],
+            "option": "boot-opts"
+        }
+    ]
+  }
+
+EQMP
+
+    {
+        .name       = "query-config-schema",
+        .args_type  = "option:s?",
+        .mhandler.cmd_new = qmp_marshal_input_query_config_schema,
+    },
 
 SQMP
 query-migrate
diff --git a/util/qemu-config.c b/util/qemu-config.c
index 01ca890..e8b4466 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -5,6 +5,7 @@ 
 #include "qapi/qmp/qerror.h"
 #include "hw/qdev.h"
 #include "qapi/error.h"
+#include "qmp-commands.h"
 
 static QemuOptsList *vm_config_groups[32];
 
@@ -37,6 +38,45 @@  QemuOptsList *qemu_find_opts(const char *group)
     return ret;
 }
 
+ConfigSchemaInfoList *qmp_query_config_schema(bool has_option,
+                                              const char *option, Error **errp)
+{
+    ConfigSchemaInfoList *conf_list = NULL, *entry;
+    ConfigSchemaInfo *info;
+    strList *str_list = NULL, *str_entry;
+    int entries, i, j;
+
+    entries = ARRAY_SIZE(vm_config_groups);
+
+    for (i = 0; i < entries; i++) {
+        if (vm_config_groups[i] != NULL &&
+            (!has_option || !strcmp(option, vm_config_groups[i]->name))) {
+            info = g_malloc0(sizeof(*info));
+            info->option = g_strdup(vm_config_groups[i]->name);
+            str_list = NULL;
+
+            for (j = 0; vm_config_groups[i]->desc[j].name != NULL; j++) {
+                str_entry = g_malloc0(sizeof(*str_entry));
+                str_entry->value = g_strdup(vm_config_groups[i]->desc[j].name);
+                str_entry->next = str_list;
+                str_list = str_entry;
+            }
+
+            info->params = str_list;
+            entry = g_malloc0(sizeof(*entry));
+            entry->value = info;
+            entry->next = conf_list;
+            conf_list = entry;
+        }
+    }
+
+    if (conf_list == NULL) {
+        error_set(errp, QERR_INVALID_OPTION_GROUP, option);
+    }
+
+    return conf_list;
+}
+
 QemuOptsList *qemu_find_opts_err(const char *group, Error **errp)
 {
     return find_list(vm_config_groups, group, errp);