Modular command line options
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.
To summarize the approach: Command line options are now organized in sets. Each set consists of a list of options, a help text, and a pointer the a parse handler that can process the options. The base option set of QEMU is automatically registered, more sets can be added via qemu_register_option_set().
The patch includes quite some refactoring in order to move the basic switches over the new scheme. So you may have a look at the data structures in qemu-common.h, but otherwise it is better to apply it and then study vl.c. More refactoring of existing switches is likely feasible, but first this needs and an OK from (or merging by) the maintainers.
--- qemu-common.h | 19 vl.c | 1690 +++++++++++++++++++++++++++++----------------------------- 2 files changed, 867 insertions(+), 842 deletions(-)
qemu-common.h +19 -0
@@ -132,4 +132,23 @@ typedef struct SerialState SerialState;typedef struct IRQState *qemu_irq ;struct pcmcia_card_s ; +#define HAS_ARG 0x0001 + +typedef struct QEMUOption { +const char *name; +int flags; +int index; +}QEMUOption ; + +typedef void QEMUOptionParser (int index,const char *optarg); + +typedef struct QEMUOptionSet { +const QEMUOption *options; +const char *help_string; +QEMUOptionParser *parse_handler; +struct QEMUOptionSet *next; +}QEMUOptionSet ; + +void qemu_register_option_set (QEMUOptionSet *option_set); +#endif
vl.c +848 -842
1,838 lines not on this page · raw message
Looks awesome.
But if you allow me to be a bit boring again, I have some extra comments.
Quoted 7 lines
qemu-common.h +6
-0+#define HAS_ARG 0x0001 + +typedef struct QEMUOption { + const char *name; + int flags; + int index;
qemu-common.h typedef struct SerialState SerialState; · line 150
Quoted 10 lines
qemu-common.h +10
-0+} QEMUOption; + +typedef void QEMUOptionParser (int index,const char *optarg); + +typedef struct QEMUOptionSet { +const QEMUOption *options; +const char *help_string; +QEMUOptionParser *parse_handler; +struct QEMUOptionSet *next; +}QEMUOptionSet ;
This is a bit counter-intuitive. If we switch to this model, the Help string would be better with the option itself, rather than the option set. Listing the options would then be a matter of iterating over each option,
and then printing each of them. An optional help string in the option set could be used as a header. Like this:
Help string for the option set:
-foo help for foo option, in a QEMUoption
-bar help for bar option, in a QEMUOption
This seems cleaner and more flexible to me. What do you think?
vl.c int inet_aton(const char *cp, struct in_ · line 151
Quoted 9 lines
#ifdef
TARGET_PPCvl.c +4
-2-#define DEFAULT_RAM_SIZE 144 +#define DEFAULT_RAM_SIZE 144 +#define DEFAULT_RAM_SIZE_STR "144"#else -#define DEFAULT_RAM_SIZE 128 +#define DEFAULT_RAM_SIZE 128 +#define DEFAULT_RAM_SIZE_STR "128"#endif
This is a bit ugly IMHO.
Defining:__STR(x) #x
STR(x) __STR(x) in a common header sounds better. We can then just STR(DEFAULT_RAM_SIZE), or whatever, in any string pasting. Since we're moving to this scheme, it is possible that new needs for doing the same thing will arise, and then just coming up with a bunch of FOO , FOO_STR binoms seems not elegant enough.
vl.c static void bdrv_format_print(void *opaq · line 5040
Quoted 4 lines
vl.c +2
-1+static int snapshot; -static int drive_init (struct drive_opt *arg,int snapshot, +static int drive_init (struct drive_opt *arg,int use_snapshot,QEMUMachine *machine)
static global variable, static function. Can't we just not use the argument at all? Yeah, I know it was there before, but...
vl.c static int main_loop(void) · line 7255
Quoted 10 lines
vl.c +9
-0+ +#ifdef CONFIG_GUS + { + "gus", + "Gravis Ultrasound GF1", + 0, + 1, + { .init_isa = GUS_init } + }, #endif
For options that are dependant on specific defines, it is better to make them separate, and then register the option set conditionally. But yeah, I know you're not doing everything in this patch. Just wanted to leave the opinion, and see whether or not you agree with it.
Other than that, seems all beautiful.
Signature · 5 lines
Glauber Costa. "Free as in Freedom" http://glommer.net "The less confident you are, the more serious you have to act."
Quoted 10 lines · Jan Kiszka
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.
To summarize the approach: Command line options are now organized in sets. Each set consists of a list of options, a help text, and a pointer the a parse handler that can process the options. The base option set of QEMU is automatically registered, more sets can be added via
qemu_register_option_set().
Quoted 7 lines
The patch includes quite some refactoring in order to move the basic switches over the new scheme. So you may have a look at the data structures in
qemu-common.h, but otherwise it is better to apply it and then studyvl.c. More refactoring of existing switches is likely feasible, but first this needs and an OK from (or merging by) the maintainers.
[...]
I am not sure it is the way to follow because soon the configuration file will be the way to pass elaborated options to QEMU.
Regards,
Fabrice.
Fabrice Bellard writes ("Re: [Qemu-devel] [PATCH 1/3] Modular command line options"):
Quoted 5 lines
Quoted 1 line · Jan Kiszka
[table-driven options]
I am not sure it is the way to follow because soon the configuration file will be the way to pass elaborated options to QEMU.
On the contrary, a table-driven command-line parsing arrangement can be reused by the config file reader. This makes it easy to ensure that the command-line options and config file settings have the same names and do the same things.
If, while inventing the new config file setup, we want to improve the names and syntaxes of the settings, we can do that if we have a way to make two differently formatted options control the same internal setting variables. But we will want one of those anyway because sooner or later we will decide that some of the config file options we have invented aren't the right way of expressing it, and will want to change them without breaking old configurations.
Using the same data source for both also has the advantage that as new syntaxes are provided in the config file, the same syntax (or some easily-related syntax, depending on the exact form of the config file and command line parser harnesses) automatically becomes available on the command line.
But I agree with Glauber Costa's specific comments and I have some of my own.
Ian.
Jan Kiszka writes ("[Qemu-devel] [PATCH 1/3] Modular command line options"):
Quoted 3 lines
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;
int int_for_handler;
void *void_for_handler;
} QEMUOption;
qemu_register_option_set(const QEMUOption *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.
Ian.
iwj writes ("Re: [Qemu-devel] [PATCH 1/3] Modular command line options"):
Quoted 5 lines
typedef void
QEMUOptionParser(struct QEMUOption *option, const char *optarg);typedef struct QEMUOption { const char *name, *helpstring; QEMUOptionParser handler;
^
This should be
QEMUOptionParser *handler;of course.
Ian.
Quoted 9 lines · Ian Jackson
Jan Kiszka writes ("[Qemu-devel] [PATCH 1/3] Modular command line options"):
Quoted 3 lines
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
Quoted 6 lines
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... :->
Quoted 2 lines
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.
Quoted 3 lines
} 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.
Quoted 13 lines
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.cyou'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 */
Quoted 5 lines
}
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.
Quoted 31 lines · Jan Kiszka
Quoted 9 lines · Ian Jackson
Jan Kiszka writes ("[Qemu-devel] [PATCH 1/3] Modular command line options"):
Quoted 3 lines
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 likeQuoted 6 lines
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... :->
Quoted 2 lines
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.
Quoted 2 lines
} QEMUOption;
Quoted 8 lines
Quoted 1 line
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?
Quoted 6 lines
Good idea. Then the structure should look like this:
struct QEMUOptionSet { const char *name;
^^^^^^^^^^^^
Signature · 5 lines
Glauber Costa. "Free as in Freedom" http://glommer.net "The less confident you are, the more serious you have to act."
Quoted 35 lines · Glauber Costa
Quoted 34 lines · Jan Kiszka
Quoted 8 lines · Ian Jackson
Jan Kiszka writes ("[Qemu-devel] [PATCH 1/3] Modular command line options"):
Quoted 3 lines
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 likeQuoted 6 lines
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... :->
Quoted 2 lines
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.
Quoted 3 lines
} QEMUOption; qemu_register_option_set(const QEMUOption *options);Here I would then suggest
qemu_register_option_set(const char
*set_name,
const QEMUOption *options);
Quoted 8 lines
Quoted 1 line
to save the chance for visually grouping options.
I don't follow the need for the
set_nameparameter. 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