[Qemu-devel] [RESEND PATCH 1/2] qapi: introduce strList and visit_type_strList()

[Qemu-devel] [RESEND PATCH 1/2] qapi: introduce strList and visit_type_strList()

[Amos Kong]

Currently we can only use ['String'] to add string to a list,
it contains some additional JSON structure.
    "multicast": [
        {
            "str": "01:80:c2:00:00:21"
        },
        {
            "str": "00:00:00:00:00:00"
        }
    ]

This patch introdued strList, we can use ['str']

    "multicast": [
        "01:00:5e:00:00:01",
        "33:33:ff:12:34:57"
    ]

Signed-off-by: Amos Kong <akong@redhat.com>
---
I used an _ugly_ "#ifndef" to limit it only be defined in qapi-types.h.
do you have some suggestion to fix it?
---
 include/qapi/visitor.h |    2 ++
 qapi/qapi-visit-core.c |   23 +++++++++++++++++++++++
 scripts/qapi-types.py  |    8 ++++++++
 3 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 1fef18c..a79ce79 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -50,6 +50,8 @@ void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp);
 void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp);
 void visit_type_bool(Visitor *v, bool *obj, const char *name, Error **errp);
 void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp);
+void visit_type_strList(Visitor *m, strList ** obj, const char *name,
+                        Error **errp);
 void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp);
 
 #endif
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 401ee6e..dc54cc8 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -257,6 +257,29 @@ void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp)
     }
 }
 
+void visit_type_strList(Visitor *m, strList ** obj, const char *name,
+                        Error **errp)
+{
+    GenericList *i, **prev = (GenericList **)obj;
+    Error *err = NULL;
+
+    if (!error_is_set(errp)) {
+        visit_start_list(m, name, &err);
+        if (!err) {
+            for (; (i = visit_next_list(m, prev, &err)) != NULL; prev = &i) {
+                strList *native_i = (strList *)i;
+                visit_type_str(m, &native_i->value, NULL, &err);
+            }
+            error_propagate(errp, err);
+            err = NULL;
+
+            /* Always call end_list if start_list succeeded.  */
+            visit_end_list(m, &err);
+        }
+        error_propagate(errp, err);
+    }
+}
+
 void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp)
 {
     if (!error_is_set(errp)) {
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 9e19920..f2ca373 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -276,6 +276,14 @@ fdecl.write(mcgen('''
 #include <stdbool.h>
 #include <stdint.h>
 
+#ifndef QGA_QAPI_TYPES_H
+typedef struct strList
+{
+  char *value;
+  struct strList *next;
+} strList;
+#endif
+
 ''',
                   guard=guardname(h_file)))
 
-- 
1.7.1

[Qemu-devel] [PATCH 2/2] monitor: introduce query-config-schema command

[Amos Kong]

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(-)

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);
-- 
1.7.1

Re: [Qemu-devel] [PATCH 2/2] monitor: introduce query-config-schema command

[Eric Blake]

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

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.

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH 2/2] monitor: introduce query-config-schema command

[Anthony Liguori]

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

[Qemu-devel] [PATCH] monitor: introduce query-config-schema command

[Amos Kong]

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.

V2: fix jaso schema and comments (Eric)

Signed-off-by: Amos Kong <akong@redhat.com>
CC: Osier Yang <jyang@redhat.com>
CC: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Amos Kong <akong@redhat.com>
---
 qapi-schema.json   | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx    | 43 ++++++++++++++++++++++++++++++++++++
 util/qemu-config.c | 51 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 158 insertions(+)

diff --git a/qapi-schema.json b/qapi-schema.json
index 751d3c2..55aee4a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3505,3 +3505,67 @@
     '*asl_compiler_rev':  'uint32',
     '*file':              'str',
     '*data':              'str' }}
+
+##
+# @ConfigParamType:
+#
+# JSON representation of values of QEMUOptionParType, may grow in future
+#
+# @flag: If no value is given, the flag is set to 1. Otherwise the value must
+#        be "on" (set to 1) or "off" (set to 0)
+#
+# @number: Simple number
+#
+# @size: The value is converted to an integer. Suffixes for kilobytes etc
+#
+# @string: Character string
+#
+# Since 1.5
+##
+{ 'enum': 'ConfigParamType',
+  'data': [ 'flag', 'number', 'size', 'string' ] }
+
+##
+# @ConfigParamInfo:
+#
+# JSON representation of QEMUOptionParameter, may grow in future
+#
+# @name: parameter name
+#
+# @type: parameter type
+#
+# @help is optional if no text was present
+#
+# Since 1.5
+##
+{ 'type': 'ConfigParamInfo',
+  'data': { 'name': 'str', 'type': 'ConfigParamType', '*help':'str' } }
+
+##
+# @ConfigSchemaInfo:
+#
+# Each command line option, and its list of parameters
+#
+# @option: option name
+#
+# @params: a list of parameters of one option
+#
+# Since 1.5
+##
+{ 'type': 'ConfigSchemaInfo',
+  'data': { 'option':'str', 'params': ['ConfigParamInfo'] } }
+
+##
+# @query-config-schema:
+#
+# Query configuration schema information
+#
+# @option: #optional option name
+#
+# 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']}
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 4d65422..19415e4 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2414,6 +2414,49 @@ EQMP
         .args_type  = "",
         .mhandler.cmd_new = qmp_marshal_input_query_uuid,
     },
+SQMP
+query-config-schema
+------------
+
+Show configuration schema.
+
+Return list of configuration schema of all options (or for the given option),
+return an error if given option doesn't exist.
+
+- "option": option name
+- "params": parameters infomation list of one option
+- "name": parameter name
+- "type": parameter type
+- "help": parameter help message
+
+Example:
+
+-> {"execute": "query-config-schema", "arguments" : {"option": "option-rom"}}
+<- {
+    "return": [
+        {
+            "params": [
+                {
+                    "name": "romfile",
+                    "type": "flag"
+                },
+                {
+                    "name": "bootindex",
+                    "type": "size"
+                }
+            ],
+            "option": "option-rom"
+        }
+    ]
+  }
+
+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..6d93642 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,56 @@ 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, *conf_entry;
+    ConfigSchemaInfo *schema_info;
+    ConfigParamInfoList *param_list = NULL, *param_entry;
+    ConfigParamInfo *param_info;
+    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))) {
+            schema_info = g_malloc0(sizeof(*schema_info));
+            schema_info->option = g_strdup(vm_config_groups[i]->name);
+            param_list = NULL;
+
+            for (j = 0; vm_config_groups[i]->desc[j].name != NULL; j++) {
+                param_info =  g_malloc0(sizeof(*param_info));
+                param_info->name = g_strdup(vm_config_groups[i]->desc[j].name);
+                param_info->type = vm_config_groups[i]->desc[j].type;
+
+                if (vm_config_groups[i]->desc[j].help) {
+                    param_info->has_help = true;
+                    param_info->help = g_strdup(
+                    vm_config_groups[i]->desc[j].help);
+                }
+
+                param_entry = g_malloc0(sizeof(*param_entry));
+                param_entry->value = param_info;
+                param_entry->next = param_list;
+                param_list = param_entry;
+            }
+
+            schema_info->params = param_list;
+            conf_entry = g_malloc0(sizeof(*conf_entry));
+            conf_entry->value = schema_info;
+            conf_entry->next = conf_list;
+            conf_list = conf_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);
-- 
1.8.1.4

[Qemu-devel] [PATCH v2] monitor: introduce query-config-schema command

[Amos Kong]

On Thu, Apr 25, 2013 at 01:33:24AM +0800, 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.
> 
> V2: fix jaso schema and comments (Eric)

Fix the subject: it's the v2, Please help to review, thanks.

---
              Amos.

Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema command

[Luiz Capitulino]

On Thu, 25 Apr 2013 01:33:24 +0800
Amos Kong <akong@redhat.com> 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.
> 
> V2: fix jaso schema and comments (Eric)

I guess this is v3, isn't it? Btw, it's better to start a new thread
when submitting a new version.

More comments below.

> Signed-off-by: Amos Kong <akong@redhat.com>
> CC: Osier Yang <jyang@redhat.com>
> CC: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  qapi-schema.json   | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx    | 43 ++++++++++++++++++++++++++++++++++++
>  util/qemu-config.c | 51 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 158 insertions(+)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 751d3c2..55aee4a 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3505,3 +3505,67 @@
>      '*asl_compiler_rev':  'uint32',
>      '*file':              'str',
>      '*data':              'str' }}
> +
> +##
> +# @ConfigParamType:
> +#
> +# JSON representation of values of QEMUOptionParType, may grow in future
> +#
> +# @flag: If no value is given, the flag is set to 1. Otherwise the value must
> +#        be "on" (set to 1) or "off" (set to 0)

Let's call this 'boolean', because it's what it is. Also, I suggest
'Accepts "on" or "off"' as the help text.

> +#
> +# @number: Simple number

Suggest "Accepts a number".

> +#
> +# @size: The value is converted to an integer. Suffixes for kilobytes etc

Suggest "Accepts a number followed by an optional postfix (K)ilo, (M)ega, (G)iga,
(T)era"

> +#
> +# @string: Character string
> +#
> +# Since 1.5
> +##
> +{ 'enum': 'ConfigParamType',
> +  'data': [ 'flag', 'number', 'size', 'string' ] }
> +
> +##
> +# @ConfigParamInfo:
> +#
> +# JSON representation of QEMUOptionParameter, may grow in future
> +#
> +# @name: parameter name
> +#
> +# @type: parameter type
> +#
> +# @help is optional if no text was present

Suggest '@help human readable text string. This text may change is not suitable
for parsing #optional'

> +#
> +# Since 1.5
> +##
> +{ 'type': 'ConfigParamInfo',
> +  'data': { 'name': 'str', 'type': 'ConfigParamType', '*help':'str' } }
> +
> +##
> +# @ConfigSchemaInfo:
> +#
> +# Each command line option, and its list of parameters
> +#
> +# @option: option name
> +#
> +# @params: a list of parameters of one option
> +#
> +# Since 1.5
> +##
> +{ 'type': 'ConfigSchemaInfo',
> +  'data': { 'option':'str', 'params': ['ConfigParamInfo'] } }
> +
> +##
> +# @query-config-schema:

If you allow me the bikeshed, I find query-config-schema too generic,
what about query-command-line-schema? query-command-line-options?

> +#
> +# Query configuration schema information
> +#
> +# @option: #optional option name
> +#
> +# 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'},

Please, let's not make option optional. It makes the code slightly more
complex for no good reason.

> + 'returns': ['ConfigSchemaInfo']}
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 4d65422..19415e4 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2414,6 +2414,49 @@ EQMP
>          .args_type  = "",
>          .mhandler.cmd_new = qmp_marshal_input_query_uuid,
>      },
> +SQMP
> +query-config-schema
> +------------
> +
> +Show configuration schema.
> +
> +Return list of configuration schema of all options (or for the given option),
> +return an error if given option doesn't exist.
> +
> +- "option": option name
> +- "params": parameters infomation list of one option
> +- "name": parameter name
> +- "type": parameter type
> +- "help": parameter help message
> +
> +Example:
> +
> +-> {"execute": "query-config-schema", "arguments" : {"option": "option-rom"}}
> +<- {
> +    "return": [
> +        {
> +            "params": [
> +                {
> +                    "name": "romfile",
> +                    "type": "flag"
> +                },
> +                {
> +                    "name": "bootindex",
> +                    "type": "size"
> +                }
> +            ],
> +            "option": "option-rom"
> +        }
> +    ]
> +  }
> +
> +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..6d93642 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,56 @@ 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, *conf_entry;
> +    ConfigSchemaInfo *schema_info;
> +    ConfigParamInfoList *param_list = NULL, *param_entry;
> +    ConfigParamInfo *param_info;
> +    int entries, i, j;
> +
> +    entries = ARRAY_SIZE(vm_config_groups);
> +
> +    for (i = 0; i < entries; i++) {

Can't you loop until vm_config_groups[i] is NULL?

> +        if (vm_config_groups[i] != NULL &&
> +            (!has_option || !strcmp(option, vm_config_groups[i]->name))) {
> +            schema_info = g_malloc0(sizeof(*schema_info));
> +            schema_info->option = g_strdup(vm_config_groups[i]->name);
> +            param_list = NULL;
> +
> +            for (j = 0; vm_config_groups[i]->desc[j].name != NULL; j++) {
> +                param_info =  g_malloc0(sizeof(*param_info));
> +                param_info->name = g_strdup(vm_config_groups[i]->desc[j].name);
> +                param_info->type = vm_config_groups[i]->desc[j].type;

That's a bug. This would only work if QemuOptType and ConfigParamType elements
ordering matched, but even then it's a bad idea to do that.

Suggest doing the type match manually via a switch().

> +
> +                if (vm_config_groups[i]->desc[j].help) {
> +                    param_info->has_help = true;
> +                    param_info->help = g_strdup(
> +                    vm_config_groups[i]->desc[j].help);
> +                }
> +
> +                param_entry = g_malloc0(sizeof(*param_entry));
> +                param_entry->value = param_info;
> +                param_entry->next = param_list;
> +                param_list = param_entry;
> +            }
> +
> +            schema_info->params = param_list;
> +            conf_entry = g_malloc0(sizeof(*conf_entry));
> +            conf_entry->value = schema_info;
> +            conf_entry->next = conf_list;
> +            conf_list = conf_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);

Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema command

[Eric Blake]

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

On 04/24/2013 12:20 PM, Luiz Capitulino wrote:
> On Thu, 25 Apr 2013 01:33:24 +0800
> Amos Kong <akong@redhat.com> 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.
>>
>> V2: fix jaso schema and comments (Eric)
> 
> I guess this is v3, isn't it? Btw, it's better to start a new thread
> when submitting a new version.

Also, move your changelog below the --- line...

> 
> More comments below.
> 
>> Signed-off-by: Amos Kong <akong@redhat.com>
>> CC: Osier Yang <jyang@redhat.com>
>> CC: Anthony Liguori <aliguori@us.ibm.com>
>> Signed-off-by: Amos Kong <akong@redhat.com>
>> ---

...here.  The changelog is essential during review, but adds nothing to
the long-term git history (a year from now, we won't care if it took 1
or 20 revisions to get to the version finally committed).

http://wiki.qemu.org/Contribute/SubmitAPatch

>>  qapi-schema.json   | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  qmp-commands.hx    | 43 ++++++++++++++++++++++++++++++++++++
>>  util/qemu-config.c | 51 +++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 158 insertions(+)
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 751d3c2..55aee4a 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -3505,3 +3505,67 @@
>>      '*asl_compiler_rev':  'uint32',
>>      '*file':              'str',
>>      '*data':              'str' }}
>> +
>> +##
>> +# @ConfigParamType:
>> +#
>> +# JSON representation of values of QEMUOptionParType, may grow in future

I see why you used this text (it's straight copy-and-paste from my email
suggestion); but it's rather geared towards someone that cares about the
internals of qemu.  On the other hand, qapi-schema is the public
interface that should tell you how to interact with qemu without having
to open include/qemu/option.h or even know about the C type
QEMUOptionParType in the first place.  Also, we've never mentioned that
enums are subject to future growth elsewhere - it's kind of implicit.  I
think the following would be better text (and sorry for leading  you in
the wrong direction; in my mail, I was trying to justify WHY I suggested
the type, not HOW it should be documented).

# Possible types for an option parameter.

>> +#
>> +# @flag: If no value is given, the flag is set to 1. Otherwise the value must
>> +#        be "on" (set to 1) or "off" (set to 0)
> 
> Let's call this 'boolean', because it's what it is. Also, I suggest
> 'Accepts "on" or "off"' as the help text.

I'm fine with calling the enum value 'boolean' even where the C code
called it 'flag'.  As long as we have a documented name that describes
the semantics of what the parameter will take, libvirt should be able to
cope.

One other concern - you document that if a flag parameter is omitted,
then it defaults to 1.  Is that really true?  For that matter, does
QEMUOptionParameter even have a way of expression which parameters are
mandatory vs. optional, and for those which are optional, what the
default value is if omitted?  If so, then we probably ought to expand
the representation of ConfigParamInfo to provide additional fields
exposing that information.

> 
>> +#
>> +# @number: Simple number
> 
> Suggest "Accepts a number".
> 
>> +#
>> +# @size: The value is converted to an integer. Suffixes for kilobytes etc
> 
> Suggest "Accepts a number followed by an optional postfix (K)ilo, (M)ega, (G)iga,
> (T)era"
> 
>> +#
>> +# @string: Character string
>> +#
>> +# Since 1.5
>> +##
>> +{ 'enum': 'ConfigParamType',
>> +  'data': [ 'flag', 'number', 'size', 'string' ] }
>> +
>> +##
>> +# @ConfigParamInfo:
>> +#
>> +# JSON representation of QEMUOptionParameter, may grow in future

Again, my fault for trying to point to the C code as my justification,
instead of providing a user-facing documentation.  I think a better
wording would be:

# Details about a single parameter of a command-line option.

>> +#
>> +# @name: parameter name
>> +#
>> +# @type: parameter type
>> +#
>> +# @help is optional if no text was present
> 
> Suggest '@help human readable text string. This text may change is not suitable
> for parsing #optional'

Well, with enough punctuation to read well:

@help: #optional human readable text string. Not suitable for parsing.

> 
>> +#
>> +# Since 1.5
>> +##
>> +{ 'type': 'ConfigParamInfo',
>> +  'data': { 'name': 'str', 'type': 'ConfigParamType', '*help':'str' } }
>> +
>> +##
>> +# @ConfigSchemaInfo:
>> +#
>> +# Each command line option, and its list of parameters

Close.  But really, a single ConfigSchemaInfo is only one command line
option, not 'each'.  I'd use:

Details about a command line option, including its list of parameters.

>> +#
>> +# @option: option name
>> +#
>> +# @params: a list of parameters of one option

No need to abbreviate in QMP; let's call this 'parameters'.

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

We aren't very consistent on whether to put space after ':' or not; I
don't care enough to fix existing uses, but it would look better if you
are at least self-consistent within a patch.

>> +
>> +##
>> +# @query-config-schema:
> 
> If you allow me the bikeshed, I find query-config-schema too generic,
> what about query-command-line-schema? query-command-line-options?

I like query-command-line-options, myself.  And if you change that name,
you should reflect the naming convention back to the other types:
s/ConfigSchemaInfo/CommandLineOptionInfo/,
s/ConfigParamInfo/CommandLineParameterInfo/,
s/ConfigParamType/CommandLineParameterType/

> 
>> +#
>> +# Query configuration schema information
>> +#
>> +# @option: #optional option name
>> +#
>> +# 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'},
> 
> Please, let's not make option optional. It makes the code slightly more
> complex for no good reason.

There's two possibilities:

1. Discovering which options are available.  Libvirt wants to do this (a
single call returns as much information as possible, and it is mandatory
for an option that was added after libvirt was compiled, but where
libvirt has ability to dynamically use the results of the introspection
to offer the user a chance to use introspected options) - for this one,
omitting 'option' is essential.

2. Querying whether a given option is available, and what it supports.
Libvirt is less likely to do this (if it needs to learn about 3
different command line options, a single call to get all options and
then read through the array is easier to code than making 3 separate QMP
calls to query about each individual option); but it makes sense from a
usability standpoint of minimizing processing time (making 3 calls that
return targeted information passes less data over the wire than making 1
call that returns all information and which the receiver must then sift
through).

Most existing query-* commands do NOT support filtering, but always
return a full array of information.  I am not opposed to filtering (I
actually think the idea of filtering is quite slick), even if libvirt
won't use it; but if you are going to simplify the command, then get rid
of 'option' entirely.  Do NOT make 'option' mandatory, as then you have
hindered the ability to learn which options exist even when you didn't
know their name a priori.

> 
>> + 'returns': ['ConfigSchemaInfo']}
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 4d65422..19415e4 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -2414,6 +2414,49 @@ EQMP
>>          .args_type  = "",
>>          .mhandler.cmd_new = qmp_marshal_input_query_uuid,
>>      },
>> +SQMP
>> +query-config-schema
>> +------------
>> +
>> +Show configuration schema.
>> +
>> +Return list of configuration schema of all options (or for the given option),
>> +return an error if given option doesn't exist.
>> +
>> +- "option": option name
>> +- "params": parameters infomation list of one option

s/infomation/information/

>> +- "name": parameter name
>> +- "type": parameter type
>> +- "help": parameter help message

I'd format this to show the levels of structures:

Return an array of command line option schema for all options (or for
the given option), returning an error if the given option doesn't exist.
 Each entry contains:
- "option": option name (json-string)
- "parameters": an array of json-objects, each describing one
                parameter of the option:
    - "name": parameter name (json-string)
    - "type": parameter type (one of 'flag', 'number', 'size',
              or 'string')
    - "help": human readable description of the parameter
              (json-string, optional)

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema command

[Eric Blake]

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

On 04/24/2013 01:39 PM, Eric Blake wrote:
> On 04/24/2013 12:20 PM, Luiz Capitulino wrote:
>> On Thu, 25 Apr 2013 01:33:24 +0800
>> Amos Kong <akong@redhat.com> 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.
>>>
>>> +##
>>> +# @ConfigParamType:
>>> +#
>>> +# JSON representation of values of QEMUOptionParType, may grow in future
> 
> I see why you used this text (it's straight copy-and-paste from my email
> suggestion); but it's rather geared towards someone that cares about the
> internals of qemu.  On the other hand, qapi-schema is the public
> interface that should tell you how to interact with qemu without having
> to open include/qemu/option.h or even know about the C type
> QEMUOptionParType in the first place.  Also, we've never mentioned that
> enums are subject to future growth elsewhere - it's kind of implicit.  I
> think the following would be better text (and sorry for leading  you in
> the wrong direction; in my mail, I was trying to justify WHY I suggested
> the type, not HOW it should be documented).

That said, while the public document need not refer to the C api, you
SHOULD patch include/qemu/option.h to document that any additions to the
C API should be reflected back into the public QMP type name.

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema command

[Amos Kong]

On Wed, Apr 24, 2013 at 01:39:08PM -0600, Eric Blake wrote:
> On 04/24/2013 12:20 PM, Luiz Capitulino wrote:
> > On Thu, 25 Apr 2013 01:33:24 +0800
> > Amos Kong <akong@redhat.com> 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.
> >>
> >> V2: fix jaso schema and comments (Eric)

...

> >> +#
> >> +# @flag: If no value is given, the flag is set to 1. Otherwise the value must
> >> +#        be "on" (set to 1) or "off" (set to 0)
> > 
> > Let's call this 'boolean', because it's what it is. Also, I suggest
> > 'Accepts "on" or "off"' as the help text.
> 
> I'm fine with calling the enum value 'boolean' even where the C code
> called it 'flag'.  As long as we have a documented name that describes
> the semantics of what the parameter will take, libvirt should be able to
> cope.
> 
> One other concern - you document that if a flag parameter is omitted,
> then it defaults to 1.  Is that really true?


I'm wrong. If it's omitted in cmdline, we will give it a default value.

example:
   enable_mlock = qemu_opt_get_bool(opts, "mlock", true);

another example:
   -boot strict=on

   bool boot_strict; (false by default)

   strict boot is disabled by default, type of strict parameter is 'QEMU_OPT_STRING'
   the logical default parameter is "off".

This kind of default info is only added in help descriptions right
now, we can add a new item 'default_value' to option.h:QemuOptDesc &
qapi-schema.json:CommandLineParameterInfo in future?

I guess the default value is useful for libvirt.

However, we live in different timezone, so I will post my latest patch
to maillist (no change about default value).

Agree with other comments, thanks.


                Amos.

Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema command

[Osier Yang]

On 25/04/13 11:52, Amos Kong wrote:
> On Wed, Apr 24, 2013 at 01:39:08PM -0600, Eric Blake wrote:
>> On 04/24/2013 12:20 PM, Luiz Capitulino wrote:
>>> On Thu, 25 Apr 2013 01:33:24 +0800
>>> Amos Kong <akong@redhat.com> 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.
>>>>
>>>> V2: fix jaso schema and comments (Eric)
> ...
>
>>>> +#
>>>> +# @flag: If no value is given, the flag is set to 1. Otherwise the value must
>>>> +#        be "on" (set to 1) or "off" (set to 0)
>>> Let's call this 'boolean', because it's what it is. Also, I suggest
>>> 'Accepts "on" or "off"' as the help text.
>> I'm fine with calling the enum value 'boolean' even where the C code
>> called it 'flag'.  As long as we have a documented name that describes
>> the semantics of what the parameter will take, libvirt should be able to
>> cope.
>>
>> One other concern - you document that if a flag parameter is omitted,
>> then it defaults to 1.  Is that really true?
>
> I'm wrong. If it's omitted in cmdline, we will give it a default value.
>
> example:
>     enable_mlock = qemu_opt_get_bool(opts, "mlock", true);
>
> another example:
>     -boot strict=on
>
>     bool boot_strict; (false by default)
>
>     strict boot is disabled by default, type of strict parameter is 'QEMU_OPT_STRING'
>     the logical default parameter is "off".

I didn't look through all the threads, not sure if it's already 
clarified, but
is the "flags" indicates whether the option is enabled or disabled by 
default?

If so, is it possible to have another "name" instead of "flag"? Or please
add documentation to tell more about the meaning.

>
> This kind of default info is only added in help descriptions right
> now, we can add a new item 'default_value' to option.h:QemuOptDesc &
> qapi-schema.json:CommandLineParameterInfo in future?

Sounds good to add it in the struct instead and dump as a JSON parameter.

>
> I guess the default value is useful for libvirt.

Yeah, sure, it's nice if libvirt could known if the device is enabled or 
disabled
by default.

Osier

Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema command

[Amos Kong]

On Thu, Apr 25, 2013 at 12:27:16PM +0800, Osier Yang wrote:
> On 25/04/13 11:52, Amos Kong wrote:
> >On Wed, Apr 24, 2013 at 01:39:08PM -0600, Eric Blake wrote:
> >>On 04/24/2013 12:20 PM, Luiz Capitulino wrote:
> >>>On Thu, 25 Apr 2013 01:33:24 +0800
> >>>Amos Kong <akong@redhat.com> 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.
> >>>>
> >>>>V2: fix jaso schema and comments (Eric)
> >...
> >
> >>>>+#
> >>>>+# @flag: If no value is given, the flag is set to 1. Otherwise the value must
> >>>>+#        be "on" (set to 1) or "off" (set to 0)
> >>>Let's call this 'boolean', because it's what it is. Also, I suggest
> >>>'Accepts "on" or "off"' as the help text.
> >>I'm fine with calling the enum value 'boolean' even where the C code
> >>called it 'flag'.  As long as we have a documented name that describes
> >>the semantics of what the parameter will take, libvirt should be able to
> >>cope.
> >>
> >>One other concern - you document that if a flag parameter is omitted,
> >>then it defaults to 1.  Is that really true?
> >
> >I'm wrong. If it's omitted in cmdline, we will give it a default value.
> >
> >example:
> >    enable_mlock = qemu_opt_get_bool(opts, "mlock", true);
> >
> >another example:
> >    -boot strict=on
> >
> >    bool boot_strict; (false by default)
> >
> >    strict boot is disabled by default, type of strict parameter is 'QEMU_OPT_STRING'
> >    the logical default parameter is "off".
> 
> I didn't look through all the threads, not sure if it's already
> clarified, but
> is the "flags" indicates whether the option is enabled or disabled
> by default?

No, it used for used to enable/disable sth, not to save default value
 
> If so, is it possible to have another "name" instead of "flag"? Or please
> add documentation to tell more about the meaning.

-- 
			Amos.

Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema command

[Luiz Capitulino]

On Thu, 25 Apr 2013 11:52:58 +0800
Amos Kong <akong@redhat.com> wrote:

> On Wed, Apr 24, 2013 at 01:39:08PM -0600, Eric Blake wrote:
> > On 04/24/2013 12:20 PM, Luiz Capitulino wrote:
> > > On Thu, 25 Apr 2013 01:33:24 +0800
> > > Amos Kong <akong@redhat.com> 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.
> > >>
> > >> V2: fix jaso schema and comments (Eric)
> 
> ...
> 
> > >> +#
> > >> +# @flag: If no value is given, the flag is set to 1. Otherwise the value must
> > >> +#        be "on" (set to 1) or "off" (set to 0)
> > > 
> > > Let's call this 'boolean', because it's what it is. Also, I suggest
> > > 'Accepts "on" or "off"' as the help text.
> > 
> > I'm fine with calling the enum value 'boolean' even where the C code
> > called it 'flag'.  As long as we have a documented name that describes
> > the semantics of what the parameter will take, libvirt should be able to
> > cope.
> > 
> > One other concern - you document that if a flag parameter is omitted,
> > then it defaults to 1.  Is that really true?
> 
> 
> I'm wrong. If it's omitted in cmdline, we will give it a default value.
> 
> example:
>    enable_mlock = qemu_opt_get_bool(opts, "mlock", true);
> 
> another example:
>    -boot strict=on
> 
>    bool boot_strict; (false by default)
> 
>    strict boot is disabled by default, type of strict parameter is 'QEMU_OPT_STRING'
>    the logical default parameter is "off".
> 
> This kind of default info is only added in help descriptions right
> now, we can add a new item 'default_value' to option.h:QemuOptDesc &
> qapi-schema.json:CommandLineParameterInfo in future?

Yes. I don't think we'll have enough time to fix this now.

> 
> I guess the default value is useful for libvirt.
> 
> However, we live in different timezone, so I will post my latest patch
> to maillist (no change about default value).
> 
> Agree with other comments, thanks.
> 
> 
>                 Amos.
> 

Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema command

[Eric Blake]

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

On 04/25/2013 06:36 AM, Luiz Capitulino wrote:

>>> One other concern - you document that if a flag parameter is omitted,
>>> then it defaults to 1.  Is that really true?
>>
>>
>> I'm wrong. If it's omitted in cmdline, we will give it a default value.
>>

>> This kind of default info is only added in help descriptions right
>> now, we can add a new item 'default_value' to option.h:QemuOptDesc &
>> qapi-schema.json:CommandLineParameterInfo in future?
> 
> Yes. I don't think we'll have enough time to fix this now.

Agreed - we've added additional information in the QMP types returned by
query-* in the past, and it's no problem if we don't expose default
values via QMP until improving the C code to track default values in
1.6.  For 1.5, let's just focus on exposing the information we have
readily available.

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema command

[Amos Kong]

On Wed, Apr 24, 2013 at 02:20:20PM -0400, Luiz Capitulino wrote:
> On Thu, 25 Apr 2013 01:33:24 +0800
> Amos Kong <akong@redhat.com> 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.
> > 
> > V2: fix jaso schema and comments (Eric)
> 
> I guess this is v3, isn't it? Btw, it's better to start a new thread
> when submitting a new version.

I didn't count RFC patch, I will use v4 for next version.
Thanks for the review.
 
> More comments below.
> 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > CC: Osier Yang <jyang@redhat.com>
> > CC: Anthony Liguori <aliguori@us.ibm.com>
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  qapi-schema.json   | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  qmp-commands.hx    | 43 ++++++++++++++++++++++++++++++++++++
> >  util/qemu-config.c | 51 +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 158 insertions(+)
> > 
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 751d3c2..55aee4a 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -3505,3 +3505,67 @@
> >      '*asl_compiler_rev':  'uint32',
> >      '*file':              'str',
> >      '*data':              'str' }}
> > +
> > +##
> > +# @ConfigParamType:
> > +#
> > +# JSON representation of values of QEMUOptionParType, may grow in future
> > +#
> > +# @flag: If no value is given, the flag is set to 1. Otherwise the value must
> > +#        be "on" (set to 1) or "off" (set to 0)
> 
> Let's call this 'boolean', because it's what it is. Also, I suggest
> 'Accepts "on" or "off"' as the help text.

Ok.

btw, using 'bool' matches with 'enum QemuOptType', but it might confuse
with 'bool' type in qapi-schema.json
 
> > +#
> > +# @number: Simple number
> 
> Suggest "Accepts a number".
> 
> > +#
> > +# @size: The value is converted to an integer. Suffixes for kilobytes etc
> 
> Suggest "Accepts a number followed by an optional postfix (K)ilo, (M)ega, (G)iga,
> (T)era"
> 
> > +#
> > +# @string: Character string
> > +#
> > +# Since 1.5
> > +##
> > +{ 'enum': 'ConfigParamType',
> > +  'data': [ 'flag', 'number', 'size', 'string' ] }
> > +
> > +##
> > +# @ConfigParamInfo:
> > +#
> > +# JSON representation of QEMUOptionParameter, may grow in future
> > +#
> > +# @name: parameter name
> > +#
> > +# @type: parameter type
> > +#
> > +# @help is optional if no text was present
> 
> Suggest '@help human readable text string. This text may change is not suitable
> for parsing #optional'
> 
> > +#
> > +# Since 1.5
> > +##
> > +{ 'type': 'ConfigParamInfo',
> > +  'data': { 'name': 'str', 'type': 'ConfigParamType', '*help':'str' } }
> > +
> > +##
> > +# @ConfigSchemaInfo:
> > +#
> > +# Each command line option, and its list of parameters
> > +#
> > +# @option: option name
> > +#
> > +# @params: a list of parameters of one option
> > +#
> > +# Since 1.5
> > +##
> > +{ 'type': 'ConfigSchemaInfo',
> > +  'data': { 'option':'str', 'params': ['ConfigParamInfo'] } }
> > +
> > +##
> > +# @query-config-schema:
> 
> If you allow me the bikeshed, I find query-config-schema too generic,
> what about query-command-line-schema? query-command-line-options?

'query-command-line-options' is clearer.
 
> > +#
> > +# Query configuration schema information
> > +#
> > +# @option: #optional option name
> > +#
> > +# 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'},
> 
> Please, let's not make option optional. It makes the code slightly more
> complex for no good reason.

For the human, if they don't know the detail name of one option, they just
list all the options, then find the useful one.

Not sure the use-case of full list for libvirt.  Osier?
 
  
....
> > +ConfigSchemaInfoList *qmp_query_config_schema(bool has_option,
> > +                                              const char *option, Error **errp)
> > +{
> > +    ConfigSchemaInfoList *conf_list = NULL, *conf_entry;
> > +    ConfigSchemaInfo *schema_info;
> > +    ConfigParamInfoList *param_list = NULL, *param_entry;
> > +    ConfigParamInfo *param_info;
> > +    int entries, i, j;
> > +
> > +    entries = ARRAY_SIZE(vm_config_groups);
> > +
> > +    for (i = 0; i < entries; i++) {
> 
> Can't you loop until vm_config_groups[i] is NULL?

Fixed.
 
> > +        if (vm_config_groups[i] != NULL &&
> > +            (!has_option || !strcmp(option, vm_config_groups[i]->name))) {
> > +            schema_info = g_malloc0(sizeof(*schema_info));
> > +            schema_info->option = g_strdup(vm_config_groups[i]->name);
> > +            param_list = NULL;
> > +
> > +            for (j = 0; vm_config_groups[i]->desc[j].name != NULL; j++) {
> > +                param_info =  g_malloc0(sizeof(*param_info));
> > +                param_info->name = g_strdup(vm_config_groups[i]->desc[j].name);

> > +                param_info->type = vm_config_groups[i]->desc[j].type;
>
> That's a bug. This would only work if QemuOptType and ConfigParamType elements
> ordering matched, but even then it's a bad idea to do that.
> 
> Suggest doing the type match manually via a switch().

Right! the order doesn't match here.

                     switch (vm_config_groups[i]->desc[j].type) {
                     case QEMU_OPT_STRING:
                         param_info->type = CONFIG_PARAM_TYPE_STRING;
                         break;
                     case QEMU_OPT_BOOL:
                         param_info->type = CONFIG_PARAM_TYPE_FLAG;  
                         break;
                     case QEMU_OPT_NUMBER:
                         param_info->type = CONFIG_PARAM_TYPE_NUMBER;               
                         break;                                                              
                     case QEMU_OPT_SIZE:                                                     
                         param_info->type = CONFIG_PARAM_TYPE_SIZE;                          
                         break;
                     }     

I think we don't need default here, until some add new items in enum
QemuOptType without update this code.


-- 
			Amos.

Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema command

[Luiz Capitulino]

On Thu, 25 Apr 2013 09:14:51 +0800
Amos Kong <akong@redhat.com> wrote:

> On Wed, Apr 24, 2013 at 02:20:20PM -0400, Luiz Capitulino wrote:
> > On Thu, 25 Apr 2013 01:33:24 +0800
> > Amos Kong <akong@redhat.com> 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.
> > > 
> > > V2: fix jaso schema and comments (Eric)
> > 
> > I guess this is v3, isn't it? Btw, it's better to start a new thread
> > when submitting a new version.
> 
> I didn't count RFC patch, I will use v4 for next version.
> Thanks for the review.
>  
> > More comments below.
> > 
> > > Signed-off-by: Amos Kong <akong@redhat.com>
> > > CC: Osier Yang <jyang@redhat.com>
> > > CC: Anthony Liguori <aliguori@us.ibm.com>
> > > Signed-off-by: Amos Kong <akong@redhat.com>
> > > ---
> > >  qapi-schema.json   | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  qmp-commands.hx    | 43 ++++++++++++++++++++++++++++++++++++
> > >  util/qemu-config.c | 51 +++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 158 insertions(+)
> > > 
> > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > index 751d3c2..55aee4a 100644
> > > --- a/qapi-schema.json
> > > +++ b/qapi-schema.json
> > > @@ -3505,3 +3505,67 @@
> > >      '*asl_compiler_rev':  'uint32',
> > >      '*file':              'str',
> > >      '*data':              'str' }}
> > > +
> > > +##
> > > +# @ConfigParamType:
> > > +#
> > > +# JSON representation of values of QEMUOptionParType, may grow in future
> > > +#
> > > +# @flag: If no value is given, the flag is set to 1. Otherwise the value must
> > > +#        be "on" (set to 1) or "off" (set to 0)
> > 
> > Let's call this 'boolean', because it's what it is. Also, I suggest
> > 'Accepts "on" or "off"' as the help text.
> 
> Ok.
> 
> btw, using 'bool' matches with 'enum QemuOptType', but it might confuse
> with 'bool' type in qapi-schema.json

I don't think so because this won't cause a real conflict, as the enum name
will always have the CONFIG_PARAM_TYPE prefix. But I 'suggested' boolean
anyway.

>  
> > > +#
> > > +# @number: Simple number
> > 
> > Suggest "Accepts a number".
> > 
> > > +#
> > > +# @size: The value is converted to an integer. Suffixes for kilobytes etc
> > 
> > Suggest "Accepts a number followed by an optional postfix (K)ilo, (M)ega, (G)iga,
> > (T)era"
> > 
> > > +#
> > > +# @string: Character string
> > > +#
> > > +# Since 1.5
> > > +##
> > > +{ 'enum': 'ConfigParamType',
> > > +  'data': [ 'flag', 'number', 'size', 'string' ] }
> > > +
> > > +##
> > > +# @ConfigParamInfo:
> > > +#
> > > +# JSON representation of QEMUOptionParameter, may grow in future
> > > +#
> > > +# @name: parameter name
> > > +#
> > > +# @type: parameter type
> > > +#
> > > +# @help is optional if no text was present
> > 
> > Suggest '@help human readable text string. This text may change is not suitable
> > for parsing #optional'
> > 
> > > +#
> > > +# Since 1.5
> > > +##
> > > +{ 'type': 'ConfigParamInfo',
> > > +  'data': { 'name': 'str', 'type': 'ConfigParamType', '*help':'str' } }
> > > +
> > > +##
> > > +# @ConfigSchemaInfo:
> > > +#
> > > +# Each command line option, and its list of parameters
> > > +#
> > > +# @option: option name
> > > +#
> > > +# @params: a list of parameters of one option
> > > +#
> > > +# Since 1.5
> > > +##
> > > +{ 'type': 'ConfigSchemaInfo',
> > > +  'data': { 'option':'str', 'params': ['ConfigParamInfo'] } }
> > > +
> > > +##
> > > +# @query-config-schema:
> > 
> > If you allow me the bikeshed, I find query-config-schema too generic,
> > what about query-command-line-schema? query-command-line-options?
> 
> 'query-command-line-options' is clearer.
>  
> > > +#
> > > +# Query configuration schema information
> > > +#
> > > +# @option: #optional option name
> > > +#
> > > +# 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'},
> > 
> > Please, let's not make option optional. It makes the code slightly more
> > complex for no good reason.
> 
> For the human, if they don't know the detail name of one option, they just
> list all the options, then find the useful one.

QMP is not concerned with human users, we can always use tools like qmp-shell
to give this feature to humans.

> Not sure the use-case of full list for libvirt.  Osier?
>  
>   
> ....
> > > +ConfigSchemaInfoList *qmp_query_config_schema(bool has_option,
> > > +                                              const char *option, Error **errp)
> > > +{
> > > +    ConfigSchemaInfoList *conf_list = NULL, *conf_entry;
> > > +    ConfigSchemaInfo *schema_info;
> > > +    ConfigParamInfoList *param_list = NULL, *param_entry;
> > > +    ConfigParamInfo *param_info;
> > > +    int entries, i, j;
> > > +
> > > +    entries = ARRAY_SIZE(vm_config_groups);
> > > +
> > > +    for (i = 0; i < entries; i++) {
> > 
> > Can't you loop until vm_config_groups[i] is NULL?
> 
> Fixed.
>  
> > > +        if (vm_config_groups[i] != NULL &&
> > > +            (!has_option || !strcmp(option, vm_config_groups[i]->name))) {
> > > +            schema_info = g_malloc0(sizeof(*schema_info));
> > > +            schema_info->option = g_strdup(vm_config_groups[i]->name);
> > > +            param_list = NULL;
> > > +
> > > +            for (j = 0; vm_config_groups[i]->desc[j].name != NULL; j++) {
> > > +                param_info =  g_malloc0(sizeof(*param_info));
> > > +                param_info->name = g_strdup(vm_config_groups[i]->desc[j].name);
> 
> > > +                param_info->type = vm_config_groups[i]->desc[j].type;
> >
> > That's a bug. This would only work if QemuOptType and ConfigParamType elements
> > ordering matched, but even then it's a bad idea to do that.
> > 
> > Suggest doing the type match manually via a switch().
> 
> Right! the order doesn't match here.
> 
>                      switch (vm_config_groups[i]->desc[j].type) {
>                      case QEMU_OPT_STRING:
>                          param_info->type = CONFIG_PARAM_TYPE_STRING;
>                          break;
>                      case QEMU_OPT_BOOL:
>                          param_info->type = CONFIG_PARAM_TYPE_FLAG;  
>                          break;
>                      case QEMU_OPT_NUMBER:
>                          param_info->type = CONFIG_PARAM_TYPE_NUMBER;               
>                          break;                                                              
>                      case QEMU_OPT_SIZE:                                                     
>                          param_info->type = CONFIG_PARAM_TYPE_SIZE;                          
>                          break;
>                      }     

Looks good.

> I think we don't need default here, until some add new items in enum
> QemuOptType without update this code.

Maybe we can have:

	default:
	 abort();

So that we catch new QEmuOpts types not accompanied by a new ConfigParamType
type.

Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema command

[Eric Blake]

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

On 04/24/2013 07:35 PM, Luiz Capitulino wrote:

>>                      case QEMU_OPT_SIZE:                                                     
>>                          param_info->type = CONFIG_PARAM_TYPE_SIZE;                          
>>                          break;
>>                      }     
> 
> Looks good.
> 
>> I think we don't need default here, until some add new items in enum
>> QemuOptType without update this code.
> 
> Maybe we can have:
> 
> 	default:
> 	 abort();
> 
> So that we catch new QEmuOpts types not accompanied by a new ConfigParamType
> type.

gcc is able to warn about missed options if you omit a default case (and
if the switch was on an actual enum type); but does not warn about
missed options if you do have a default case.  Libvirt has actually been
switching to default-free coding styles on any switch statement where we
expect an enum will grow in the future, for that reason.

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema command

[Amos Kong]

On Wed, Apr 24, 2013 at 07:44:37PM -0600, Eric Blake wrote:
> On 04/24/2013 07:35 PM, Luiz Capitulino wrote:
> 
> >>                      case QEMU_OPT_SIZE:                                                     
> >>                          param_info->type = CONFIG_PARAM_TYPE_SIZE;                          
> >>                          break;
> >>                      }     
> > 
> > Looks good.
> > 
> >> I think we don't need default here, until some add new items in enum
> >> QemuOptType without update this code.
> > 
> > Maybe we can have:
> > 
> > 	default:
> > 	 abort();
> > 
> > So that we catch new QEmuOpts types not accompanied by a new ConfigParamType
> > type.
> 
> gcc is able to warn about missed options if you omit a default case (and
> if the switch was on an actual enum type); but does not warn about
> missed options if you do have a default case.  Libvirt has actually been
> switching to default-free coding styles on any switch statement where we
> expect an enum will grow in the future, for that reason.

util/qemu-config.c: In function ‘qmp_query_command_line_options’:
util/qemu-config.c:60:17: error: enumeration value ‘QEMU_OPT_NEW’ not
handled in switch [-Werror=switch]
cc1: all warnings being treated as errors

Good! no need default case here.

-- 
			Amos.

Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema command

[Luiz Capitulino]

On Wed, 24 Apr 2013 19:44:37 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 04/24/2013 07:35 PM, Luiz Capitulino wrote:
> 
> >>                      case QEMU_OPT_SIZE:                                                     
> >>                          param_info->type = CONFIG_PARAM_TYPE_SIZE;                          
> >>                          break;
> >>                      }     
> > 
> > Looks good.
> > 
> >> I think we don't need default here, until some add new items in enum
> >> QemuOptType without update this code.
> > 
> > Maybe we can have:
> > 
> > 	default:
> > 	 abort();
> > 
> > So that we catch new QEmuOpts types not accompanied by a new ConfigParamType
> > type.
> 
> gcc is able to warn about missed options if you omit a default case (and
> if the switch was on an actual enum type); but does not warn about
> missed options if you do have a default case.  Libvirt has actually been
> switching to default-free coding styles on any switch statement where we
> expect an enum will grow in the future, for that reason.

Ah, didn't know that. Fine with me then.

Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema command

[Eric Blake]

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

On 04/24/2013 07:14 PM, Amos Kong wrote:
>>> +##
>>> +{'command': 'query-config-schema', 'data': {'*option': 'str'},
>>
>> Please, let's not make option optional. It makes the code slightly more
>> complex for no good reason.
> 
> For the human, if they don't know the detail name of one option, they just
> list all the options, then find the useful one.
> 
> Not sure the use-case of full list for libvirt.  Osier?

I think I already answered that - having the whole list (single QMP
call) is more efficient than calling multiple times per command we are
interested in.  Also, most other query-* commands do NOT filter; having
the 'option' argument introduces filtering.  It's up to Luiz whether we
leave it as is, or omit '*option' for now and save filtering for later,
but I'd rather not have mandatory 'option' as then you have to know in
advance which command line options you care about.

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema command

[Osier Yang]

On 25/04/13 09:14, Amos Kong wrote:
> On Wed, Apr 24, 2013 at 02:20:20PM -0400, Luiz Capitulino wrote:
>> On Thu, 25 Apr 2013 01:33:24 +0800
>> Amos Kong <akong@redhat.com> 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.
>>>
>>> V2: fix jaso schema and comments (Eric)
>> I guess this is v3, isn't it? Btw, it's better to start a new thread
>> when submitting a new version.
> I didn't count RFC patch, I will use v4 for next version.
> Thanks for the review.
>   
>> More comments below.
>>
>>> Signed-off-by: Amos Kong <akong@redhat.com>
>>> CC: Osier Yang <jyang@redhat.com>
>>> CC: Anthony Liguori <aliguori@us.ibm.com>
>>> Signed-off-by: Amos Kong <akong@redhat.com>
>>> ---
>>>   qapi-schema.json   | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   qmp-commands.hx    | 43 ++++++++++++++++++++++++++++++++++++
>>>   util/qemu-config.c | 51 +++++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 158 insertions(+)
>>>
>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>> index 751d3c2..55aee4a 100644
>>> --- a/qapi-schema.json
>>> +++ b/qapi-schema.json
>>> @@ -3505,3 +3505,67 @@
>>>       '*asl_compiler_rev':  'uint32',
>>>       '*file':              'str',
>>>       '*data':              'str' }}
>>> +
>>> +##
>>> +# @ConfigParamType:
>>> +#
>>> +# JSON representation of values of QEMUOptionParType, may grow in future
>>> +#
>>> +# @flag: If no value is given, the flag is set to 1. Otherwise the value must
>>> +#        be "on" (set to 1) or "off" (set to 0)
>> Let's call this 'boolean', because it's what it is. Also, I suggest
>> 'Accepts "on" or "off"' as the help text.
> Ok.
>
> btw, using 'bool' matches with 'enum QemuOptType', but it might confuse
> with 'bool' type in qapi-schema.json
>   
>>> +#
>>> +# @number: Simple number
>> Suggest "Accepts a number".
>>
>>> +#
>>> +# @size: The value is converted to an integer. Suffixes for kilobytes etc
>> Suggest "Accepts a number followed by an optional postfix (K)ilo, (M)ega, (G)iga,
>> (T)era"
>>
>>> +#
>>> +# @string: Character string
>>> +#
>>> +# Since 1.5
>>> +##
>>> +{ 'enum': 'ConfigParamType',
>>> +  'data': [ 'flag', 'number', 'size', 'string' ] }
>>> +
>>> +##
>>> +# @ConfigParamInfo:
>>> +#
>>> +# JSON representation of QEMUOptionParameter, may grow in future
>>> +#
>>> +# @name: parameter name
>>> +#
>>> +# @type: parameter type
>>> +#
>>> +# @help is optional if no text was present
>> Suggest '@help human readable text string. This text may change is not suitable
>> for parsing #optional'
>>
>>> +#
>>> +# Since 1.5
>>> +##
>>> +{ 'type': 'ConfigParamInfo',
>>> +  'data': { 'name': 'str', 'type': 'ConfigParamType', '*help':'str' } }
>>> +
>>> +##
>>> +# @ConfigSchemaInfo:
>>> +#
>>> +# Each command line option, and its list of parameters
>>> +#
>>> +# @option: option name
>>> +#
>>> +# @params: a list of parameters of one option
>>> +#
>>> +# Since 1.5
>>> +##
>>> +{ 'type': 'ConfigSchemaInfo',
>>> +  'data': { 'option':'str', 'params': ['ConfigParamInfo'] } }
>>> +
>>> +##
>>> +# @query-config-schema:
>> If you allow me the bikeshed, I find query-config-schema too generic,
>> what about query-command-line-schema? query-command-line-options?
> 'query-command-line-options' is clearer.
>   
>>> +#
>>> +# Query configuration schema information
>>> +#
>>> +# @option: #optional option name
>>> +#
>>> +# 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'},
>> Please, let's not make option optional. It makes the code slightly more
>> complex for no good reason.
> For the human, if they don't know the detail name of one option, they just
> list all the options, then find the useful one.
>
> Not sure the use-case of full list for libvirt.  Osier?

One of the use is to query what options qemu support. Changing the
"option" to be mandatory is not nice? any other way for libvirt to query
what options qemu support?

>   
>    
> ....
>>> +ConfigSchemaInfoList *qmp_query_config_schema(bool has_option,
>>> +                                              const char *option, Error **errp)
>>> +{
>>> +    ConfigSchemaInfoList *conf_list = NULL, *conf_entry;
>>> +    ConfigSchemaInfo *schema_info;
>>> +    ConfigParamInfoList *param_list = NULL, *param_entry;
>>> +    ConfigParamInfo *param_info;
>>> +    int entries, i, j;
>>> +
>>> +    entries = ARRAY_SIZE(vm_config_groups);
>>> +
>>> +    for (i = 0; i < entries; i++) {
>> Can't you loop until vm_config_groups[i] is NULL?
> Fixed.
>   
>>> +        if (vm_config_groups[i] != NULL &&
>>> +            (!has_option || !strcmp(option, vm_config_groups[i]->name))) {
>>> +            schema_info = g_malloc0(sizeof(*schema_info));
>>> +            schema_info->option = g_strdup(vm_config_groups[i]->name);
>>> +            param_list = NULL;
>>> +
>>> +            for (j = 0; vm_config_groups[i]->desc[j].name != NULL; j++) {
>>> +                param_info =  g_malloc0(sizeof(*param_info));
>>> +                param_info->name = g_strdup(vm_config_groups[i]->desc[j].name);
>>> +                param_info->type = vm_config_groups[i]->desc[j].type;
>> That's a bug. This would only work if QemuOptType and ConfigParamType elements
>> ordering matched, but even then it's a bad idea to do that.
>>
>> Suggest doing the type match manually via a switch().
> Right! the order doesn't match here.
>
>                       switch (vm_config_groups[i]->desc[j].type) {
>                       case QEMU_OPT_STRING:
>                           param_info->type = CONFIG_PARAM_TYPE_STRING;
>                           break;
>                       case QEMU_OPT_BOOL:
>                           param_info->type = CONFIG_PARAM_TYPE_FLAG;
>                           break;
>                       case QEMU_OPT_NUMBER:
>                           param_info->type = CONFIG_PARAM_TYPE_NUMBER;
>                           break;
>                       case QEMU_OPT_SIZE:
>                           param_info->type = CONFIG_PARAM_TYPE_SIZE;
>                           break;
>                       }
>
> I think we don't need default here, until some add new items in enum
> QemuOptType without update this code.
>
>

gcc will give warnings if the new enum member is added but not handled
in the switch. From this p.o.v, not using the default is good.

Re: [Qemu-devel] [RESEND PATCH 1/2] qapi: introduce strList and visit_type_strList()

[Eric Blake]

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

On 04/24/2013 06:47 AM, Amos Kong wrote:
> Currently we can only use ['String'] to add string to a list,
> it contains some additional JSON structure.
>     "multicast": [
>         {
>             "str": "01:80:c2:00:00:21"
>         },
>         {
>             "str": "00:00:00:00:00:00"
>         }
>     ]
> 
> This patch introdued strList, we can use ['str']
> 
>     "multicast": [
>         "01:00:5e:00:00:01",
>         "33:33:ff:12:34:57"
>     ]
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
> I used an _ugly_ "#ifndef" to limit it only be defined in qapi-types.h.
> do you have some suggestion to fix it?

I'm not the maintainer, so you may still want to wait for a better
suggestion.  But since I personally don't mind the #ifdef if that's what
it takes to get this to compile, and since I'd like to see the feature
added, I'm fine giving:

Reviewed-by: Eric Blake <eblake@redhat.com>

That said, I think the API in patch 2/2 is not quite right; and when you
fix things there, you may not need this patch in the 1.5 timeframe...

-- 
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: 621 bytes --]

Re: [Qemu-devel] [RESEND PATCH 1/2] qapi: introduce strList and visit_type_strList()

[Amos Kong]

On Wed, Apr 24, 2013 at 08:01:50AM -0600, Eric Blake wrote:
> On 04/24/2013 06:47 AM, Amos Kong wrote:
> > Currently we can only use ['String'] to add string to a list,
> > it contains some additional JSON structure.
> >     "multicast": [
> >         {
> >             "str": "01:80:c2:00:00:21"
> >         },
> >         {
> >             "str": "00:00:00:00:00:00"
> >         }
> >     ]
> > 
> > This patch introdued strList, we can use ['str']
> > 
> >     "multicast": [
> >         "01:00:5e:00:00:01",
> >         "33:33:ff:12:34:57"
> >     ]
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> > I used an _ugly_ "#ifndef" to limit it only be defined in qapi-types.h.
> > do you have some suggestion to fix it?
> 
> I'm not the maintainer, so you may still want to wait for a better
> suggestion.  But since I personally don't mind the #ifdef if that's what
> it takes to get this to compile, and since I'd like to see the feature
> added, I'm fine giving:
 
I will send a v2 to solve this issue.

> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> That said, I think the API in patch 2/2 is not quite right; and when you
> fix things there, you may not need this patch in the 1.5 timeframe...

I already saw your comment, will split those two patches.
Thanks

-- 
			Amos.

Re: [Qemu-devel] [RESEND PATCH 1/2] qapi: introduce strList and visit_type_strList()

[Eric Blake]

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

On 04/24/2013 06:47 AM, Amos Kong wrote:
> Currently we can only use ['String'] to add string to a list,
> it contains some additional JSON structure.

Side note - when sending a 2-patch series, it helps to include a 0/2
cover letter.

-- 
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: 621 bytes --]