On Fri, Mar 07, 2014 at 10:54:09AM +0100, Markus Armbruster wrote:
> Eric Blake <eblake@xxxxxxxxxx> 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'.
Hi Markus,
> 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.
'-cdrom' is the 'other argument' == 'Non-parameter argument'?
We can use a enum state.
| { 'enum': 'ArgumentStateType',
| 'data': ['no-argument', 'unspecified-parameters-argument',
| 'non-parameter-argument']
| }
|
| { 'type': 'CommandLineOptionInfo',
| 'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'],
| '*argument-state': 'ArgumentStateType' } }
> parameters is [] unless it's a QemuOpts argument. Then it lists the
> recognized parameters.
How about balloon? we should treat as 'taking unspecified parameters'?
"-balloon none disable balloon device\n"
"-balloon virtio[,addr=str]\n"
I think we can only check this from help message in qemu-options.hx,
is it a stable/acceptable method?
We introduce query-command-line-options command to avoid libvirt to
check qemu commandline information by scanning qemu's help message,
it's not strict & stable.
> 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'm confused here. But there is only one option (-drive), we should
return merged desc table (legacy + common).
Desc table of qemu_drive_opts is NULL, then -drive can also take
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.
Thanks, Amos
> >> 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@xxxxxxxxxx>
> >> ---
> >> 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;
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list