Re: [libvirt] [Qemu-devel] [PATCH v5 for 2.0 3/3] abort QEMU if group name in option table doesn't match with defined option name

On 03/28/2014 08:55 AM, Markus Armbruster wrote: > Amos Kong <akong(a)redhat.com&gt; 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.