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