Re: [Qemu-devel] Re: [PATCH 1/3] Modular command line options

[Glauber Costa]

On Thu, May 22, 2008 at 9:49 AM, Jan Kiszka <address@hidden> 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?

>
>
> Good idea. Then the structure should look like this:
>
> struct QEMUOptionSet {
>        const char *name;
          ^^^^^^^^^^^^


-- 
Glauber Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."