| Message ID | 1395925471-19841-4-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
Hi Amos, On Thu, Mar 27, 2014 at 09:04:31PM +0800, Amos Kong wrote: > All the options are defined in qemu-options.hx. If we can't find a > matched option definition by group name of option table, then the > group name doesn't match with defined option name, it's not allowed > from 2.0 > > Signed-off-by: Amos Kong <akong@redhat.com> > --- > qemu-options.h | 12 ++++++++++++ > util/qemu-config.c | 28 ++++++++++++++++++++++++++++ > vl.c | 19 ++----------------- > 3 files changed, 42 insertions(+), 17 deletions(-) > > diff --git a/qemu-options.h b/qemu-options.h > index 89a009e..4024487 100644 > --- a/qemu-options.h > +++ b/qemu-options.h > @@ -28,9 +28,21 @@ > #ifndef _QEMU_OPTIONS_H_ > #define _QEMU_OPTIONS_H_ > > +#include "sysemu/arch_init.h" > + > enum { > #define QEMU_OPTIONS_GENERATE_ENUM > #include "qemu-options-wrapper.h" > }; > > +#define HAS_ARG 0x0001 > + > +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 f610101..eba5428 100644 > --- a/util/qemu-config.c > +++ b/util/qemu-config.c > @@ -6,6 +6,14 @@ > #include "hw/qdev.h" > #include "qapi/error.h" > #include "qmp-commands.h" > +#include "qemu-options.h" > + > +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]; > @@ -184,6 +192,20 @@ void qemu_add_drive_opts(QemuOptsList *list) > abort(); > } > > +/* check if the option is defined in qemu-options.hx */ > +static bool opt_is_defined(const char *name) > +{ > + int i; > + > + for (i = 0; qemu_options[i].name; i++) { > + if (!strcmp(qemu_options[i].name, name)) { > + return true; > + } > + } > + > + return false; > +} > + > void qemu_add_opts(QemuOptsList *list) > { > int entries, i; > @@ -193,6 +215,12 @@ void qemu_add_opts(QemuOptsList *list) > for (i = 0; i < entries; i++) { > if (vm_config_groups[i] == NULL) { > vm_config_groups[i] = list; > + if (!opt_is_defined(list->name)) { > + error_report("Didn't find a matched option definition, " > + "group name (%s) of option table must match with " > + "defined option name (Since 2.0)", list->name); I'm not a native English speaker but I don't fell comfortable with this message output, what about "option table's group name (%s) must match with predefined option name (Since 2.0)"? The patch looks good, I tested it and it works properly, so Reviewed-by: Leandro Dorileo <l@dorileo.org> > + abort(); > + } > return; > } > } > diff --git a/vl.c b/vl.c > index 0464494..bd44c52 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" > @@ -2082,22 +2081,6 @@ static void help(int exitcode) > exit(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"); > @@ -2842,6 +2825,8 @@ static const QEMUOption *lookup_opt(int argc, char **argv, > return popt; > } > > +#undef HAS_ARG > + > static gpointer malloc_and_trace(gsize n_bytes) > { > void *ptr = malloc(n_bytes); > -- > 1.8.5.3 > >
Amos Kong <akong@redhat.com> writes: > All the options are defined in qemu-options.hx. If we can't find a > matched option definition by group name of option table, then the > group name doesn't match with defined option name, it's not allowed > from 2.0 > > Signed-off-by: Amos Kong <akong@redhat.com> > --- > qemu-options.h | 12 ++++++++++++ > util/qemu-config.c | 28 ++++++++++++++++++++++++++++ > vl.c | 19 ++----------------- > 3 files changed, 42 insertions(+), 17 deletions(-) > > diff --git a/qemu-options.h b/qemu-options.h > index 89a009e..4024487 100644 > --- a/qemu-options.h > +++ b/qemu-options.h > @@ -28,9 +28,21 @@ > #ifndef _QEMU_OPTIONS_H_ > #define _QEMU_OPTIONS_H_ > > +#include "sysemu/arch_init.h" > + > enum { > #define QEMU_OPTIONS_GENERATE_ENUM > #include "qemu-options-wrapper.h" > }; > > +#define HAS_ARG 0x0001 > + > +typedef struct QEMUOption { > + const char *name; > + int flags; > + int index; > + uint32_t arch_mask; > +} QEMUOption; > + > +extern const QEMUOption qemu_options[]; > #endif Instead of exposing struct QEMUOption and qemu_options[] here, why not simply put opt_is_defined() in vl.c, and declare it in qemu-options.h? > diff --git a/util/qemu-config.c b/util/qemu-config.c > index f610101..eba5428 100644 > --- a/util/qemu-config.c > +++ b/util/qemu-config.c > @@ -6,6 +6,14 @@ > #include "hw/qdev.h" > #include "qapi/error.h" > #include "qmp-commands.h" > +#include "qemu-options.h" > + > +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]; > @@ -184,6 +192,20 @@ void qemu_add_drive_opts(QemuOptsList *list) > abort(); > } > > +/* check if the option is defined in qemu-options.hx */ > +static bool opt_is_defined(const char *name) > +{ > + int i; > + > + for (i = 0; qemu_options[i].name; i++) { > + if (!strcmp(qemu_options[i].name, name)) { > + return true; > + } > + } > + > + return false; > +} > + The same loop is hidden in lookup_opt(). We could avoid the duplication by putting it in a function returning the option index. That could easily be a proper enumeration type, like this, in qemu-options.h: typedef enum QEMUOption { #define QEMU_OPTIONS_GENERATE_ENUM #include "qemu-options-wrapper.h" } QEMUOption; QEMUOption qemu_option_lookup(const char *name); plus a rename of vl.c's QEMUOption to QEMUOptionDesc or something. Can be done on top. > void qemu_add_opts(QemuOptsList *list) > { > int entries, i; > @@ -193,6 +215,12 @@ void qemu_add_opts(QemuOptsList *list) > for (i = 0; i < entries; i++) { > if (vm_config_groups[i] == NULL) { > vm_config_groups[i] = list; > + if (!opt_is_defined(list->name)) { > + error_report("Didn't find a matched option definition, " > + "group name (%s) of option table must match with " > + "defined option name (Since 2.0)", list->name); > + abort(); > + } > return; > } > } Simple! Wish it was my idea ;) Why not simply assert(opt_is_defined(list->name))? > diff --git a/vl.c b/vl.c > index 0464494..bd44c52 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" > @@ -2082,22 +2081,6 @@ static void help(int exitcode) > exit(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"); > @@ -2842,6 +2825,8 @@ static const QEMUOption *lookup_opt(int argc, char **argv, > return popt; > } > > +#undef HAS_ARG > + > static gpointer malloc_and_trace(gsize n_bytes) > { > void *ptr = malloc(n_bytes); Undefining HAS_ARG here, where it hasn't done any harm, while letting it pollute every other compilation unit including qemu-options.h makes no sense.
On 03/28/2014 08:55 AM, Markus Armbruster wrote: > Amos Kong <akong@redhat.com> writes: > >> All the options are defined in qemu-options.hx. If we can't find a >> matched option definition by group name of option table, then the >> group name doesn't match with defined option name, it's not allowed >> from 2.0 >> >> @@ -193,6 +215,12 @@ void qemu_add_opts(QemuOptsList *list) >> for (i = 0; i < entries; i++) { >> if (vm_config_groups[i] == NULL) { >> vm_config_groups[i] = list; >> + if (!opt_is_defined(list->name)) { >> + error_report("Didn't find a matched option definition, " >> + "group name (%s) of option table must match with " >> + "defined option name (Since 2.0)", list->name); >> + abort(); >> + } >> return; >> } >> } > > Simple! Wish it was my idea ;) > > Why not simply assert(opt_is_defined(list->name))? Indeed, using assert() would also solve the problem of the error message being awkward. >> >> -#define HAS_ARG 0x0001 >> - >> - >> -static const QEMUOption qemu_options[] = { >> - { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL }, >> -#define QEMU_OPTIONS_GENERATE_OPTIONS >> -#include "qemu-options-wrapper.h" >> - { NULL }, >> -}; >> >> +#undef HAS_ARG HAS_ARG is not very namespace clean. Prior to your patch, it was used only in a single file (where we know it doesn't collide). After your patch, it is now in a header used by multiple files. >> + >> static gpointer malloc_and_trace(gsize n_bytes) >> { >> void *ptr = malloc(n_bytes); > > Undefining HAS_ARG here, where it hasn't done any harm, while letting it > pollute every other compilation unit including qemu-options.h makes no > sense. Maybe a better approach would be to create an enum in qemu-options.h of actual flag values: typedef enum { QEMU_OPT_HAS_ARG = 1, } QEMUOptionFlags; and use QEMU_OPT_HAS_ARG instead of HAS_ARG in vl.c. Additionally, you either have to s/HAS_ARG/QEMU_OPT_HAS_ARG/ throughout the .hx file, or you can take a shortcut in qemu-config.c: #define HAS_ARG QEMU_OPT_HAS_ARG const QEMUOption qemu_options[] = { { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL }, #define QEMU_OPTIONS_GENERATE_OPTIONS #include "qemu-options-wrapper.h" { NULL }, }; #undef HAS_ARG since that is the only place that includes the .hx file at a point where HAS_ARG has to be expanded to something useful.
Eric Blake <eblake@redhat.com> writes: > On 03/28/2014 08:55 AM, Markus Armbruster wrote: >> Amos Kong <akong@redhat.com> writes: >> >>> All the options are defined in qemu-options.hx. If we can't find a >>> matched option definition by group name of option table, then the >>> group name doesn't match with defined option name, it's not allowed >>> from 2.0 >>> > >>> @@ -193,6 +215,12 @@ void qemu_add_opts(QemuOptsList *list) >>> for (i = 0; i < entries; i++) { >>> if (vm_config_groups[i] == NULL) { >>> vm_config_groups[i] = list; >>> + if (!opt_is_defined(list->name)) { >>> + error_report("Didn't find a matched option definition, " >>> + "group name (%s) of option table must match with " >>> + "defined option name (Since 2.0)", list->name); >>> + abort(); >>> + } >>> return; >>> } >>> } >> >> Simple! Wish it was my idea ;) >> >> Why not simply assert(opt_is_defined(list->name))? > > Indeed, using assert() would also solve the problem of the error message > being awkward. > >>> >>> -#define HAS_ARG 0x0001 >>> - > >>> - >>> -static const QEMUOption qemu_options[] = { >>> - { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL }, >>> -#define QEMU_OPTIONS_GENERATE_OPTIONS >>> -#include "qemu-options-wrapper.h" >>> - { NULL }, >>> -}; > >>> >>> +#undef HAS_ARG > > HAS_ARG is not very namespace clean. Prior to your patch, it was used > only in a single file (where we know it doesn't collide). After your > patch, it is now in a header used by multiple files. > >>> + >>> static gpointer malloc_and_trace(gsize n_bytes) >>> { >>> void *ptr = malloc(n_bytes); >> >> Undefining HAS_ARG here, where it hasn't done any harm, while letting it >> pollute every other compilation unit including qemu-options.h makes no >> sense. > > Maybe a better approach would be to create an enum in qemu-options.h of > actual flag values: > > typedef enum { > QEMU_OPT_HAS_ARG = 1, > } QEMUOptionFlags; > > and use QEMU_OPT_HAS_ARG instead of HAS_ARG in vl.c. Additionally, you > either have to s/HAS_ARG/QEMU_OPT_HAS_ARG/ throughout the .hx file, or > you can take a shortcut in qemu-config.c: > > #define HAS_ARG QEMU_OPT_HAS_ARG > > const QEMUOption qemu_options[] = { > { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL }, > #define QEMU_OPTIONS_GENERATE_OPTIONS > #include "qemu-options-wrapper.h" > { NULL }, > }; > > #undef HAS_ARG > > since that is the only place that includes the .hx file at a point where > HAS_ARG has to be expanded to something useful. Either something like this, or avoid moving this stuff to a header. I prefer the latter, and suggested how in my review.
diff --git a/qemu-options.h b/qemu-options.h index 89a009e..4024487 100644 --- a/qemu-options.h +++ b/qemu-options.h @@ -28,9 +28,21 @@ #ifndef _QEMU_OPTIONS_H_ #define _QEMU_OPTIONS_H_ +#include "sysemu/arch_init.h" + enum { #define QEMU_OPTIONS_GENERATE_ENUM #include "qemu-options-wrapper.h" }; +#define HAS_ARG 0x0001 + +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 f610101..eba5428 100644 --- a/util/qemu-config.c +++ b/util/qemu-config.c @@ -6,6 +6,14 @@ #include "hw/qdev.h" #include "qapi/error.h" #include "qmp-commands.h" +#include "qemu-options.h" + +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]; @@ -184,6 +192,20 @@ void qemu_add_drive_opts(QemuOptsList *list) abort(); } +/* check if the option is defined in qemu-options.hx */ +static bool opt_is_defined(const char *name) +{ + int i; + + for (i = 0; qemu_options[i].name; i++) { + if (!strcmp(qemu_options[i].name, name)) { + return true; + } + } + + return false; +} + void qemu_add_opts(QemuOptsList *list) { int entries, i; @@ -193,6 +215,12 @@ void qemu_add_opts(QemuOptsList *list) for (i = 0; i < entries; i++) { if (vm_config_groups[i] == NULL) { vm_config_groups[i] = list; + if (!opt_is_defined(list->name)) { + error_report("Didn't find a matched option definition, " + "group name (%s) of option table must match with " + "defined option name (Since 2.0)", list->name); + abort(); + } return; } } diff --git a/vl.c b/vl.c index 0464494..bd44c52 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" @@ -2082,22 +2081,6 @@ static void help(int exitcode) exit(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"); @@ -2842,6 +2825,8 @@ static const QEMUOption *lookup_opt(int argc, char **argv, return popt; } +#undef HAS_ARG + static gpointer malloc_and_trace(gsize n_bytes) { void *ptr = malloc(n_bytes);
All the options are defined in qemu-options.hx. If we can't find a matched option definition by group name of option table, then the group name doesn't match with defined option name, it's not allowed from 2.0 Signed-off-by: Amos Kong <akong@redhat.com> --- qemu-options.h | 12 ++++++++++++ util/qemu-config.c | 28 ++++++++++++++++++++++++++++ vl.c | 19 ++----------------- 3 files changed, 42 insertions(+), 17 deletions(-)