Re: [Qemu-devel] [PATCH v4 2/2] query-command-line-options: query all the options in qemu-options.hx

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 03/05/2014 07:36 PM, Amos Kong wrote:
>> vm_config_groups[] only contains part of the options which have
>> argument, and all options which have no argument aren't added
>> to vm_config_groups[]. Current query-command-line-options only
>> checks options from vm_config_groups[], so some options will
>> be lost.
>> 
>> We have macro in qemu-options.hx to generate a table that
>> contains all the options. This patch tries to query options
>> from the table.
>> 
>> Then we won't lose the legacy options that weren't added to
>> vm_config_groups[] (eg: -vnc, -smbios). The options that have
>> no argument will also be returned (eg: -enable-fips)
>> 
>> Some options that have argument have a NULL desc list, some
>> options don't have argument, and "parameters" is mandatory
>> in the past. So we add a new field "argument" to present
>> if the option takes unspecified arguments.
>
> I like Markus' suggestion of naming the new field
> 'unspecified-parameters' rather than 'argument'.

Looking again, there are more problems.

1. Non-parameter argument vs. parameter argument taking unspecified
   parameters

   Example: -device takes unspecified parameters.  -cdrom doesn't take
   parameters, it takes a file name.  Yet, the command reports the same
   for both: "parameters": [], "argument": true.

   Looks like we need a tri-state: option takes no argument, QemuOpts
   argument, or other argument.

   parameters is [] unless it's a QemuOpts argument.  Then it lists the
   recognized parameters.

2. Our dear friend -drive is more complicated than you might think

   We special-case it to report the union of drive_config_groups[],
   which contains qemu_legacy_drive_opts, qemu_common_drive_opts and
   qemu_drive_opts.  The latter accepts unspecified parameters.

   I believe qemu_drive_opts is actually not used by the (complex!) code
   parsing the argument of -drive.

   Nevertheless, said code accepts more than just qemu_legacy_drive_opts
   and qemu_common_drive_opts, namely driver-specific parameters.

   Until we define those properly in a schema, I guess the best we can
   do is add one more case: option takes QemuOpts argument, but
   parameters is not exhaustive.

>> This patch also fixes options to match their actual command-line
>> spelling rather than an alternate name associated with the
>> option table in use by the command.
>
> Should we independently patch hw/acpi/core.c to rename qemu_acpi_opts
> from "acpi" to "acpitable" to match the command line option?  Same for
> vl.c and qemu_boot_opts from "boot-opts" to "boot"?  Same for vl.c and
> qemu_smp_opts from "smp-opts" to "smp"?  Those were the obvious
> mismatches I found where the command line was spelled differently than
> the vm_config_groups entry.

Without such a change, the command lies, because it fails to connect the
option to its QemuOptsList.  Example:

    {"parameters": [], "option": "acpitable", "argument": true},

However, the vm_config_groups[].name values are ABI: they're the section
names recognized by -readconfig and produced by -writeconfig.  Thus,
this is an incompatible change.  It's also an improvement of sorts:
things become more consistent.

We could avoid it with a suitable mapping from option name to option
group name.  Simplest way to do that is store only the exceptions from
the rule "the names are the same".

Do we care?

> This is a bug fix patch, so let's shoot to get it into 2.0.

Yes.

>> Signed-off-by: Amos Kong <akong@redhat.com>
>> ---
>>  qapi-schema.json   |  8 ++++++--
>>  qemu-options.h     | 10 ++++++++++
>>  util/qemu-config.c | 44 ++++++++++++++++++++++++++++++++++++++------
>>  vl.c               | 15 ---------------
>>  4 files changed, 54 insertions(+), 23 deletions(-)
>
>> 
>> +++ b/util/qemu-config.c
>> @@ -6,6 +6,16 @@
>>  #include "hw/qdev.h"
>>  #include "qapi/error.h"
>>  #include "qmp-commands.h"
>> +#include "qemu-options.h"
>> +
>> +#define HAS_ARG 0x0001
>
> Hmm, we are now duplicating this macro between here and vl.c.  I'd
> prefer it gets hoisted into the .h file, so that it doesn't get out of
> sync between the two clients.

Flag values should be defined next to the flags type or variable, in
this case next to QEMUOption.  The patch hoists QEMUOption vl.c to
qemu-options.h.

It does that because it spreads option handling to another file.
Before, it's all in vl.c.  After, qemu_options[] and
qmp_query_command_line_options() are in qemu-config.c, and lookup_opts()
stays in vl.c.  Not thrilled by that.  Can we keep it in one place?

qemu-config.c isn't about the command line.  I suspect
qmp_query_command_line_options() ended up there just because its
problematic design choice *not* to work on command line options, but on
vm_config_groups[].  This series fixes that design choice.

We can't simply move the new qmp_query_command_line_options() out,
because it still uses vm_config_groups[], which is static.  I take that
as a sign of us not having gotten the interfaces quite right, yet.

I append a quick sketch.  It needs a bit more work to fully separate
qmp_query_command_line_options() and the helpers dealing with
CommandLineParameterInfoList from the QemuOpts code.

Since 2.0 is closing in fast, I won't insist on clean separation now.
We can do that on top.



diff --git a/util/qemu-config.c b/util/qemu-config.c
index 2f89b92..45f48da 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -49,7 +49,7 @@ QemuOptsList *qemu_find_opts(const char *group)
     return ret;
 }
 
-static CommandLineParameterInfoList *get_param_infolist(const QemuOptDesc *desc)
+static CommandLineParameterInfoList *get_param_info(const QemuOptDesc *desc)
 {
     CommandLineParameterInfoList *param_list = NULL, *entry;
     CommandLineParameterInfo *info;
@@ -88,17 +88,6 @@ static CommandLineParameterInfoList *get_param_infolist(const QemuOptDesc *desc)
     return param_list;
 }
 
-static int get_group_index(const char *name)
-{
-    int i;
-
-    for (i = 0; vm_config_groups[i] != NULL; i++) {
-        if (!strcmp(vm_config_groups[i]->name, name)) {
-            return i;
-        }
-    }
-    return -1;
-}
 /* remove repeated entry from the info list */
 static void cleanup_infolist(CommandLineParameterInfoList *head)
 {
@@ -134,16 +123,16 @@ static void connect_infolist(CommandLineParameterInfoList *head,
 }
 
 /* access all the local QemuOptsLists for drive option */
-static CommandLineParameterInfoList *get_drive_infolist(void)
+static CommandLineParameterInfoList *get_drive_param_info(void)
 {
     CommandLineParameterInfoList *head = NULL, *cur;
     int i;
 
     for (i = 0; drive_config_groups[i] != NULL; i++) {
         if (!head) {
-            head = get_param_infolist(drive_config_groups[i]->desc);
+            head = get_param_info(drive_config_groups[i]->desc);
         } else {
-            cur = get_param_infolist(drive_config_groups[i]->desc);
+            cur = get_param_info(drive_config_groups[i]->desc);
             connect_infolist(head, cur);
         }
     }
@@ -159,28 +148,25 @@ CommandLineOptionInfoList *qmp_query_command_line_options(bool has_option,
     CommandLineOptionInfoList *conf_list = NULL, *entry;
     CommandLineOptionInfo *info;
     int i;
+    QemuOptsList *list;
 
     for (i = 0; qemu_options[i].name; i++) {
         if (!has_option || !strcmp(option, qemu_options[i].name)) {
             info = g_malloc0(sizeof(*info));
             info->option = g_strdup(qemu_options[i].name);
 
-            int idx = get_group_index(qemu_options[i].name);
-
-            if (qemu_options[i].flags) {
-                info->argument = true;
-            }
-
             if (!strcmp("drive", qemu_options[i].name)) {
-                info->parameters = get_drive_infolist();
-            } else if (idx >= 0) {
-                info->parameters =
-                    get_param_infolist(vm_config_groups[idx]->desc);
+                info->parameters = get_drive_param_info();
+            } else {
+                list = qemu_find_opts_err(qemu_options[i].name, NULL);
+                info->parameters = list ? get_param_info(list->desc) : NULL;
             }
 
             if (!info->parameters) {
                 info->has_argument = true;
+                info->argument = qemu_options[i].flags;
             }
+
             entry = g_malloc0(sizeof(*entry));
             entry->value = info;
             entry->next = conf_list;