| Message ID | 1394073416-12578-3-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
Amos Kong <akong@redhat.com> writes: > 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. > > 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. > > 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(-) > > diff --git a/qapi-schema.json b/qapi-schema.json > index 193e7e4..4ab0d16 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -4070,12 +4070,16 @@ > # > # @option: option name > # > -# @parameters: an array of @CommandLineParameterInfo > +# @parameters: array of @CommandLineParameterInfo, possibly empty > +# @argument: @optional present if the @parameters array is empty. If > +# true, then the option takes unspecified arguments, if > +# false, then the option is merely a boolean flag (since 2.0) Suggest "if false, then the option takes no argument". I'd call it something like 'unspecified-parameters' to emphasize the connection with parameters. > # > # Since 1.5 > ## > { 'type': 'CommandLineOptionInfo', > - 'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'] } } > + 'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'], > + '*argument': 'bool' } } > > ## > # @query-command-line-options: > diff --git a/qemu-options.h b/qemu-options.h > index 89a009e..8a515e0 100644 > --- a/qemu-options.h > +++ b/qemu-options.h > @@ -25,6 +25,8 @@ > * THE SOFTWARE. > */ > > +#include "sysemu/arch_init.h" > + Why do you need this header? If you need it, you should #include within the multiple-inclusion guard! > #ifndef _QEMU_OPTIONS_H_ > #define _QEMU_OPTIONS_H_ > > @@ -33,4 +35,12 @@ enum { > #include "qemu-options-wrapper.h" > }; > > +typedef struct QEMUOption { > + const char *name; > + int flags; > + int index; > + uint32_t arch_mask; > +} QEMUOption; > + > +extern const QEMUOption qemu_options[]; > #endif > diff --git a/util/qemu-config.c b/util/qemu-config.c > index d2facfd..2f89b92 100644 > --- a/util/qemu-config.c > +++ 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 > + > +const QEMUOption qemu_options[] = { > + { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL }, > +#define QEMU_OPTIONS_GENERATE_OPTIONS > +#include "qemu-options-wrapper.h" > + { NULL }, > +}; > > static QemuOptsList *vm_config_groups[32]; > static QemuOptsList *drive_config_groups[4]; > @@ -78,6 +88,17 @@ 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) > { > @@ -139,15 +160,26 @@ CommandLineOptionInfoList *qmp_query_command_line_options(bool has_option, > CommandLineOptionInfo *info; > int i; > > - for (i = 0; vm_config_groups[i] != NULL; i++) { > - if (!has_option || !strcmp(option, vm_config_groups[i]->name)) { > + 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(vm_config_groups[i]->name); > - if (!strcmp("drive", vm_config_groups[i]->name)) { > + info->option = g_strdup(qemu_options[i].name); > + > + int idx = get_group_index(qemu_options[i].name); Style nit: declaration after statement. Suggest to declare it together with i above, as "int i, idx;". > + > + if (qemu_options[i].flags) { > + info->argument = true; > + } The existing code using flags tests flags & HAS_ARG, see lookup_opt()). Suggest to stick to that for consistency. Please move the assigment next to the assignment to info->has_argument, as shown below. > + > + if (!strcmp("drive", qemu_options[i].name)) { > info->parameters = get_drive_infolist(); > - } else { > + } else if (idx >= 0) { > info->parameters = > - get_param_infolist(vm_config_groups[i]->desc); > + get_param_infolist(vm_config_groups[idx]->desc); > + } > + > + if (!info->parameters) { > + info->has_argument = true; info->argument = qemu_options[i].flags & HAS_ARG; > } > entry = g_malloc0(sizeof(*entry)); > entry->value = info; > diff --git a/vl.c b/vl.c > index 685a7a9..6269762 100644 > --- a/vl.c > +++ b/vl.c > @@ -111,7 +111,6 @@ int main(int argc, char **argv) > #include "trace/control.h" > #include "qemu/queue.h" > #include "sysemu/cpus.h" > -#include "sysemu/arch_init.h" > #include "qemu/osdep.h" > > #include "ui/qemu-spice.h" > @@ -1992,20 +1991,6 @@ static void help(int exitcode) > > #define HAS_ARG 0x0001 > > -typedef struct QEMUOption { > - const char *name; > - int flags; > - int index; > - uint32_t arch_mask; > -} QEMUOption; > - > -static const QEMUOption qemu_options[] = { > - { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL }, > -#define QEMU_OPTIONS_GENERATE_OPTIONS > -#include "qemu-options-wrapper.h" > - { NULL }, > -}; > - > static bool vga_available(void) > { > return object_class_by_name("VGA") || object_class_by_name("isa-vga");
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'. > > 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. This is a bug fix patch, so let's shoot to get it into 2.0. > > 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.
On Thu, Mar 06, 2014 at 02:23:15PM -0700, Eric Blake wrote: > 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'. > > > > > 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"? Yes, we should. > Those were the obvious > mismatches I found where the command line was spelled differently than > the vm_config_groups entry. > > This is a bug fix patch, so let's shoot to get it into 2.0. > > > > > 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. > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
Amos Kong <akong@redhat.com> writes: > 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. > > 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. [...] > diff --git a/util/qemu-config.c b/util/qemu-config.c > index d2facfd..2f89b92 100644 > --- a/util/qemu-config.c > +++ 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 > + > +const QEMUOption qemu_options[] = { > + { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL }, > +#define QEMU_OPTIONS_GENERATE_OPTIONS > +#include "qemu-options-wrapper.h" > + { NULL }, > +}; > > static QemuOptsList *vm_config_groups[32]; > static QemuOptsList *drive_config_groups[4]; > @@ -78,6 +88,17 @@ 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) > { Please separate functions by an empty line. Actually, please drop get_group_index() and use existing qemu_find_opts_err(name, NULL). [...]
diff --git a/qapi-schema.json b/qapi-schema.json index 193e7e4..4ab0d16 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -4070,12 +4070,16 @@ # # @option: option name # -# @parameters: an array of @CommandLineParameterInfo +# @parameters: array of @CommandLineParameterInfo, possibly empty +# @argument: @optional present if the @parameters array is empty. If +# true, then the option takes unspecified arguments, if +# false, then the option is merely a boolean flag (since 2.0) # # Since 1.5 ## { 'type': 'CommandLineOptionInfo', - 'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'] } } + 'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'], + '*argument': 'bool' } } ## # @query-command-line-options: diff --git a/qemu-options.h b/qemu-options.h index 89a009e..8a515e0 100644 --- a/qemu-options.h +++ b/qemu-options.h @@ -25,6 +25,8 @@ * THE SOFTWARE. */ +#include "sysemu/arch_init.h" + #ifndef _QEMU_OPTIONS_H_ #define _QEMU_OPTIONS_H_ @@ -33,4 +35,12 @@ enum { #include "qemu-options-wrapper.h" }; +typedef struct QEMUOption { + const char *name; + int flags; + int index; + uint32_t arch_mask; +} QEMUOption; + +extern const QEMUOption qemu_options[]; #endif diff --git a/util/qemu-config.c b/util/qemu-config.c index d2facfd..2f89b92 100644 --- a/util/qemu-config.c +++ 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 + +const QEMUOption qemu_options[] = { + { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL }, +#define QEMU_OPTIONS_GENERATE_OPTIONS +#include "qemu-options-wrapper.h" + { NULL }, +}; static QemuOptsList *vm_config_groups[32]; static QemuOptsList *drive_config_groups[4]; @@ -78,6 +88,17 @@ 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) { @@ -139,15 +160,26 @@ CommandLineOptionInfoList *qmp_query_command_line_options(bool has_option, CommandLineOptionInfo *info; int i; - for (i = 0; vm_config_groups[i] != NULL; i++) { - if (!has_option || !strcmp(option, vm_config_groups[i]->name)) { + 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(vm_config_groups[i]->name); - if (!strcmp("drive", vm_config_groups[i]->name)) { + 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 { + } else if (idx >= 0) { info->parameters = - get_param_infolist(vm_config_groups[i]->desc); + get_param_infolist(vm_config_groups[idx]->desc); + } + + if (!info->parameters) { + info->has_argument = true; } entry = g_malloc0(sizeof(*entry)); entry->value = info; diff --git a/vl.c b/vl.c index 685a7a9..6269762 100644 --- a/vl.c +++ b/vl.c @@ -111,7 +111,6 @@ int main(int argc, char **argv) #include "trace/control.h" #include "qemu/queue.h" #include "sysemu/cpus.h" -#include "sysemu/arch_init.h" #include "qemu/osdep.h" #include "ui/qemu-spice.h" @@ -1992,20 +1991,6 @@ static void help(int exitcode) #define HAS_ARG 0x0001 -typedef struct QEMUOption { - const char *name; - int flags; - int index; - uint32_t arch_mask; -} QEMUOption; - -static const QEMUOption qemu_options[] = { - { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL }, -#define QEMU_OPTIONS_GENERATE_OPTIONS -#include "qemu-options-wrapper.h" - { NULL }, -}; - static bool vga_available(void) { return object_class_by_name("VGA") || object_class_by_name("isa-vga");
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. 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. 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(-)