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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2685 bytes --]

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    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]