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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 03/28/2014 08:55 AM, Markus Armbruster wrote:
>> Amos Kong <address@hidden> 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.