Glauber Costa wrote:
> On Thu, May 22, 2008 at 9:49 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> Ian Jackson wrote:
>>> Jan Kiszka writes ("[Qemu-devel] [PATCH 1/3] Modular command line options"):
>>>> Following up on my earlier proposal to introduce per-machine command
>>>> line options, this version provides a more generic approach. It should
>>>> also be usable for scenarios like per-arch or per-accelerator.
>>> I approve of splitting the code up like this, and having a
>>> table-driven parsing arrangement.  But ideally we could get rid of
>>> `index' and the giant switch() statements too.  Something more like
>>>
>>>   typedef void QEMUOptionParser(struct QEMUOption *option, const char *optarg);
>>>
>>>   typedef struct QEMUOption {
>>>       const char *name, *helpstring;
>>>       QEMUOptionParser handler;
>>>       int flags;
>> Ack. This just enforces a bit more effort to convert the existing
>> opts... :->
>>
>>>       int int_for_handler;
>>>       void *void_for_handler;
>> I don't think there is an need for both. A plain
>>
>>        void *parser_opaque;
>>
>> should suffice as the user can perfectly typecast the void to int.
>>
>>>   } QEMUOption;
>>>
>>>   qemu_register_option_set(const QEMUOption *options);
>> Here I would then suggest
>>
>>        qemu_register_option_set(const char *set_name,
>>                                 const QEMUOption *options);
>>
>> to save the chance for visually grouping options.
> 
> I don't follow the need for the set_name parameter. This should be in
> the OptionSet structure.
> You mean, joining two groups if they are registered with the same
> name? This can be done by enforcing them to have
> the same "name" parameter in the OptionSet structure. Am I failing to
> understand anything here?

Let's consider this hypothetical scenario:

$ qemu -h
...
Options specific to KQEMU accelerator:
-kernel-kqemu   enable KQEMU full virtualization (default is user mode only)
-no-kqemu       disable KQEMU kernel module usage
...

The registration will work like this:

QEMUOption kqemu_options[] = {
	{ "kernel-kqemu", "enable KQEMU...", kqemu_enable_kernel, 0, NULL },
	{ "no-kqemu", "disable KQEMU...", kqemu_disable, 0, NULL },
	{ NULL }
};
...
cpu_get_phys_page_debug("KQEMU accelerator", kqemu_options);

The set name is then kept in the _private_ QEMUOptionSet structure that
is created and hooked into the global list by cpu_get_phys_page_debug.

Joining groups with identical names, at least for the help output, was
not planned yet. Might be a useful extension, but I would wait for
feedback on use cases first.

Jan