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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Eric Blake <eblake@xxxxxxxxxx> writes:

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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]