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

[Jan Kiszka <jan.kiszka@web.de>]

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

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.

> 
> We pass the QEMUOption* to the parser handler so it can see the
> canonical name and any extra stuff put in the option structure.
> and in vl.c you'd do something like this:
> 
>   static const QEMUOption basic_options[]= {
>     ...
>     { "hda", opthandler_drive, 0, 0 },
>     { "hdb", opthandler_drive, 0, 1 },
>     { "hdc", opthandler_drive, 0, 3 },
>     { "hdd", opthandler_drive, 0, 4 },
>     ...
>     { 0 } /* null entry is required to terminate the table */
>   }
> 
>     qemu_register_option_set(basic_options);
> 
> The linked list of option tables is private to the option parser.

Good idea. Then the structure should look like this:

struct QEMUOptionSet {
	const char *name;
	const QEMUOption *options;
	struct QEMUOptionSet *next;
};


OK, as this version would require even more refactoring, please let us
agree on the critical data structures first. Specifically, this takes an
ack from the maintainers.

Jan


PS: The element set of a future config file format could perfectly grow
with each QEMUOption registered to the core. So I also see no conflict
of this effort with the config file specification work.


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