| Message ID | 1333111003-28556-1-git-send-email-liwp@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Mar 30, 2012 at 08:36:43PM +0800, Wanpeng Li wrote: > Consider of the options parse process in main function of vl.c is too > long.It should be module into single function to clear ideas, strengthen > the source code management, and increase code readability.So I module the > process of options parse as function options_parse, and expose some variables > in order to not influence command-line invocations. > > Signed-off-by: Wanpeng Li <liwp@linux.vnet.ibm.com> > --- > vl.c | 159 ++++++++++++++++++++++++++++++++++------------------------------- > 1 files changed, 83 insertions(+), 76 deletions(-) > > diff --git a/vl.c b/vl.c > index 0fccf50..fa4d0a9 100644 > --- a/vl.c > +++ b/vl.c > @@ -2251,84 +2251,40 @@ int qemu_init_main_loop(void) > return main_loop_init(); > } > > -int main(int argc, char **argv, char **envp) > -{ > - int i; > - int snapshot, linux_boot; > - const char *icount_option = NULL; > - const char *initrd_filename; > - const char *kernel_filename, *kernel_cmdline; > - char boot_devices[33] = "cad"; /* default to HD->floppy->CD-ROM */ > - DisplayState *ds; > - DisplayChangeListener *dcl; > - int cyls, heads, secs, translation; > - QemuOpts *hda_opts = NULL, *opts, *machine_opts; > - QemuOptsList *olist; > - int optind; > - const char *optarg; > - const char *loadvm = NULL; > - QEMUMachine *machine; > - const char *cpu_model; > - const char *vga_model = NULL; > - const char *pid_file = NULL; > - const char *incoming = NULL; > +int snapshot, linux_boot; > +const char *icount_option; > +const char *initrd_filename; > +const char *kernel_filename, *kernel_cmdline; > +char boot_devices[33] = "cad"; /* default to HD->floppy->CD-ROM */ > +DisplayState *ds; > +DisplayChangeListener *dcl; > +int cyls, heads, secs, translation; > +QemuOpts *hda_opts , *opts, *machine_opts; > +QemuOptsList *olist; > +int optind; > +const char *loadvm; > +QEMUMachine *machine; > +const char *cpu_model; > +const char *vga_model; > +const char *pid_file; > +const char *incoming; > #ifdef CONFIG_VNC > - int show_vnc_port = 0; > +int show_vnc_port; [snip] > +int defconfig = 1; > +const char *log_mask; > +const char *log_file; > +GMemVTable mem_trace = { > + .malloc = malloc_and_trace, > + .realloc = realloc_and_trace, > + .free = free_and_trace, > +}; > +const char *trace_events; > +const char *trace_file; > > +static void options_parse(int argc, char **argv) > +{ While code modularization is a worthy goal, I don't think this patch is really an improvement. QEMU already has far too many adhoc global variables, without adding another 30 or more. The resulting code isn't even simplified or more readable IMHO, it is merely different. Daniel
On 03/30/2012 07:36 AM, Wanpeng Li wrote: > Consider of the options parse process in main function of vl.c is too > long.It should be module into single function to clear ideas, strengthen > the source code management, and increase code readability.So I module the > process of options parse as function options_parse, and expose some variables > in order to not influence command-line invocations. > > Signed-off-by: Wanpeng Li<liwp@linux.vnet.ibm.com> I've got pending patches that significantly refactor this code. http://mid.gmane.org/1332169763-30665-1-git-send-email-aliguori@us.ibm.com I think I'll drop some of the more controversial patches and resubmit soon. Regards, Anthony Liguori
On Fri, Mar 30, 2012 at 01:53:14PM +0100, Daniel P. Berrange wrote: >On Fri, Mar 30, 2012 at 08:36:43PM +0800, Wanpeng Li wrote: >> Consider of the options parse process in main function of vl.c is too >> long.It should be module into single function to clear ideas, strengthen >> the source code management, and increase code readability.So I module the >> process of options parse as function options_parse, and expose some variables >> in order to not influence command-line invocations. >> >> Signed-off-by: Wanpeng Li <liwp@linux.vnet.ibm.com> >> --- >> vl.c | 159 ++++++++++++++++++++++++++++++++++------------------------------- >> 1 files changed, 83 insertions(+), 76 deletions(-) >> >> diff --git a/vl.c b/vl.c >> index 0fccf50..fa4d0a9 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -2251,84 +2251,40 @@ int qemu_init_main_loop(void) >> return main_loop_init(); >> } >> >> -int main(int argc, char **argv, char **envp) >> -{ >> - int i; >> - int snapshot, linux_boot; >> - const char *icount_option = NULL; >> - const char *initrd_filename; >> - const char *kernel_filename, *kernel_cmdline; >> - char boot_devices[33] = "cad"; /* default to HD->floppy->CD-ROM */ >> - DisplayState *ds; >> - DisplayChangeListener *dcl; >> - int cyls, heads, secs, translation; >> - QemuOpts *hda_opts = NULL, *opts, *machine_opts; >> - QemuOptsList *olist; >> - int optind; >> - const char *optarg; >> - const char *loadvm = NULL; >> - QEMUMachine *machine; >> - const char *cpu_model; >> - const char *vga_model = NULL; >> - const char *pid_file = NULL; >> - const char *incoming = NULL; >> +int snapshot, linux_boot; >> +const char *icount_option; >> +const char *initrd_filename; >> +const char *kernel_filename, *kernel_cmdline; >> +char boot_devices[33] = "cad"; /* default to HD->floppy->CD-ROM */ >> +DisplayState *ds; >> +DisplayChangeListener *dcl; >> +int cyls, heads, secs, translation; >> +QemuOpts *hda_opts , *opts, *machine_opts; >> +QemuOptsList *olist; >> +int optind; >> +const char *loadvm; >> +QEMUMachine *machine; >> +const char *cpu_model; >> +const char *vga_model; >> +const char *pid_file; >> +const char *incoming; >> #ifdef CONFIG_VNC >> - int show_vnc_port = 0; >> +int show_vnc_port; > >[snip] > >> +int defconfig = 1; >> +const char *log_mask; >> +const char *log_file; >> +GMemVTable mem_trace = { >> + .malloc = malloc_and_trace, >> + .realloc = realloc_and_trace, >> + .free = free_and_trace, >> +}; >> +const char *trace_events; >> +const char *trace_file; >> >> +static void options_parse(int argc, char **argv) >> +{ > >While code modularization is a worthy goal, I don't think this patch is >really an improvement. QEMU already has far too many adhoc global variables, >without adding another 30 or more. The resulting code isn't even simplified >or more readable IMHO, it is merely different. > >Daniel There are about 856 lines of codes handle options parse in main function. It is ugly and reduce readability. So I module these codes to a single function called "options_parse".Since there are amounts of command_line parameters which lead to must transfer many parameters to function options_parse, so I expose some variables to global in order to handler this issue. Regards, Wanpeng Li
On Fri, Mar 30, 2012 at 09:25:47PM +0800, Wanpeng Li wrote: > On Fri, Mar 30, 2012 at 01:53:14PM +0100, Daniel P. Berrange wrote: > >On Fri, Mar 30, 2012 at 08:36:43PM +0800, Wanpeng Li wrote: > >> Consider of the options parse process in main function of vl.c is too > >> long.It should be module into single function to clear ideas, strengthen > >> the source code management, and increase code readability.So I module the > >> process of options parse as function options_parse, and expose some variables > >> in order to not influence command-line invocations. > >> > >> Signed-off-by: Wanpeng Li <liwp@linux.vnet.ibm.com> > >> --- > >> vl.c | 159 ++++++++++++++++++++++++++++++++++------------------------------- > >> 1 files changed, 83 insertions(+), 76 deletions(-) > >> > >> diff --git a/vl.c b/vl.c > >> index 0fccf50..fa4d0a9 100644 > >> --- a/vl.c > >> +++ b/vl.c > >> @@ -2251,84 +2251,40 @@ int qemu_init_main_loop(void) > >> return main_loop_init(); > >> } > >> > >> -int main(int argc, char **argv, char **envp) > >> -{ > >> - int i; > >> - int snapshot, linux_boot; > >> - const char *icount_option = NULL; > >> - const char *initrd_filename; > >> - const char *kernel_filename, *kernel_cmdline; > >> - char boot_devices[33] = "cad"; /* default to HD->floppy->CD-ROM */ > >> - DisplayState *ds; > >> - DisplayChangeListener *dcl; > >> - int cyls, heads, secs, translation; > >> - QemuOpts *hda_opts = NULL, *opts, *machine_opts; > >> - QemuOptsList *olist; > >> - int optind; > >> - const char *optarg; > >> - const char *loadvm = NULL; > >> - QEMUMachine *machine; > >> - const char *cpu_model; > >> - const char *vga_model = NULL; > >> - const char *pid_file = NULL; > >> - const char *incoming = NULL; > >> +int snapshot, linux_boot; > >> +const char *icount_option; > >> +const char *initrd_filename; > >> +const char *kernel_filename, *kernel_cmdline; > >> +char boot_devices[33] = "cad"; /* default to HD->floppy->CD-ROM */ > >> +DisplayState *ds; > >> +DisplayChangeListener *dcl; > >> +int cyls, heads, secs, translation; > >> +QemuOpts *hda_opts , *opts, *machine_opts; > >> +QemuOptsList *olist; > >> +int optind; > >> +const char *loadvm; > >> +QEMUMachine *machine; > >> +const char *cpu_model; > >> +const char *vga_model; > >> +const char *pid_file; > >> +const char *incoming; > >> #ifdef CONFIG_VNC > >> - int show_vnc_port = 0; > >> +int show_vnc_port; > > > >[snip] > > > >> +int defconfig = 1; > >> +const char *log_mask; > >> +const char *log_file; > >> +GMemVTable mem_trace = { > >> + .malloc = malloc_and_trace, > >> + .realloc = realloc_and_trace, > >> + .free = free_and_trace, > >> +}; > >> +const char *trace_events; > >> +const char *trace_file; > >> > >> +static void options_parse(int argc, char **argv) > >> +{ > > > >While code modularization is a worthy goal, I don't think this patch is > >really an improvement. QEMU already has far too many adhoc global variables, > >without adding another 30 or more. The resulting code isn't even simplified > >or more readable IMHO, it is merely different. > > > >Daniel > > There are about 856 lines of codes handle options parse in main function. > It is ugly and reduce readability. So I module these codes to a > single function called "options_parse".Since there are amounts of > command_line parameters which lead to must transfer many parameters to function > options_parse, so I expose some variables to global in order to handler > this issue. Anthony's aforementioned patches avoid the new globals by moving them into a single global configuration struct. Keeps things a little tidier and makes is easier to recognize when code is accessing a global config option. > > Regards, > Wanpeng Li > > -- > LTC China, IBM, Shanghai > >
Wanpeng Li writes: > Consider of the options parse process in main function of vl.c is too > long.It should be module into single function to clear ideas, strengthen > the source code management, and increase code readability.So I module the > process of options parse as function options_parse, and expose some variables > in order to not influence command-line invocations. Another approach would be to use something similar to QOM type registration. In order to keep code to a minimum, you can define an array of pointers to QParameter structures, instead of an array of function pointers (what __attribute__((constructors)) does). So, for example: static void opt_icount_handler(const char *args) { if (kvm_enabled() || xen_enabled()) { fprintf(stderr, "-icount is not allowed with kvm or xen\n"); exit(1); } /* ... contents of configure_icount ... */ } static QEMUOption opt_icount = { .name = "icount", .signature = "[N|auto]", .help = "enable virtual instruction counter with 2^N clock ticks per instruction", .mode = QEMU_MODE_SYSTEM, .priority = QEMU_OPT_PRIO_BEFORE_DEVICES, .section = QEMU_OPT_SECTION_DEBUG, .handler = opt_icount_handler }; param_register(opt_icount); Here, param_register would define an array of pointers to QEMUOption structures, although I'm not sure how this can be handled on formats other than ELF. This is mostly what you already have in QEMUOptionParameter, and could still be automatically generated from an extended qemu-options.hx when necessary; so I'm talking about extending it to also contain: * Priority; a set of predefined priorities can be provided, so that you just decide the partial order of your option. * Handler function. * QEMU mode(s) that provide this option; this could also be extended to establish target restrictions (e.g., only available in system-x86_64). This might help improving the current situation by having the code where it logically belongs (e.g., HW-specific options registered along the code for that HW), or might just add more cruft to it... Lluis
On 03/30/2012 10:59 AM, Lluís Vilanova wrote: > Wanpeng Li writes: > >> Consider of the options parse process in main function of vl.c is too >> long.It should be module into single function to clear ideas, strengthen >> the source code management, and increase code readability.So I module the >> process of options parse as function options_parse, and expose some variables >> in order to not influence command-line invocations. > > Another approach would be to use something similar to QOM type registration. > > In order to keep code to a minimum, you can define an array of pointers to > QParameter structures, instead of an array of function pointers (what > __attribute__((constructors)) does). > > So, for example: > > static void opt_icount_handler(const char *args) > { > if (kvm_enabled() || xen_enabled()) { > fprintf(stderr, "-icount is not allowed with kvm or xen\n"); > exit(1); > } > /* ... contents of configure_icount ... */ > } > > static QEMUOption opt_icount = { > .name = "icount", > .signature = "[N|auto]", > .help = "enable virtual instruction counter with 2^N clock ticks per instruction", > .mode = QEMU_MODE_SYSTEM, > .priority = QEMU_OPT_PRIO_BEFORE_DEVICES, > .section = QEMU_OPT_SECTION_DEBUG, > .handler = opt_icount_handler > }; > > param_register(opt_icount); > > > Here, param_register would define an array of pointers to QEMUOption structures, > although I'm not sure how this can be handled on formats other than ELF. My plan is to have a -query-capabilities so we can change the help text, then use GOptionContext. It would take a small function that goes through and promotes any option in the format '-foo' to '--foo'. But that's not too hard. Then we can completely rip out the option parsing, qemu-options.hx, and all of that other junk. Regards, Anthony Liguori
On 03/30/2012 11:09 AM, Anthony Liguori wrote: > On 03/30/2012 10:59 AM, Lluís Vilanova wrote: >> Wanpeng Li writes: >> >>> Consider of the options parse process in main function of vl.c is too >>> long.It should be module into single function to clear ideas, strengthen >>> the source code management, and increase code readability.So I module the >>> process of options parse as function options_parse, and expose some variables >>> in order to not influence command-line invocations. >> >> Another approach would be to use something similar to QOM type registration. >> >> In order to keep code to a minimum, you can define an array of pointers to >> QParameter structures, instead of an array of function pointers (what >> __attribute__((constructors)) does). >> >> So, for example: >> >> static void opt_icount_handler(const char *args) >> { >> if (kvm_enabled() || xen_enabled()) { >> fprintf(stderr, "-icount is not allowed with kvm or xen\n"); >> exit(1); >> } >> /* ... contents of configure_icount ... */ >> } >> >> static QEMUOption opt_icount = { >> .name = "icount", >> .signature = "[N|auto]", >> .help = "enable virtual instruction counter with 2^N clock ticks per >> instruction", >> .mode = QEMU_MODE_SYSTEM, >> .priority = QEMU_OPT_PRIO_BEFORE_DEVICES, >> .section = QEMU_OPT_SECTION_DEBUG, >> .handler = opt_icount_handler >> }; >> >> param_register(opt_icount); >> >> >> Here, param_register would define an array of pointers to QEMUOption structures, >> although I'm not sure how this can be handled on formats other than ELF. > > My plan is to have a -query-capabilities so we can change the help text, then > use GOptionContext. > > It would take a small function that goes through and promotes any option in the > format '-foo' to '--foo'. But that's not too hard. > > Then we can completely rip out the option parsing, qemu-options.hx, and all of > that other junk. glib's option parsing support has some really nice features like translation support, grouping, and inline help for option definition. The grouping is a really nice touch. It will let us have a target specific group, device groups, etc. Regards, Anthony Liguori > > Regards, > > Anthony Liguori >
Anthony Liguori writes: >> My plan is to have a -query-capabilities so we can change the help text, then >> use GOptionContext. >> >> It would take a small function that goes through and promotes any option in the >> format '-foo' to '--foo'. But that's not too hard. >> >> Then we can completely rip out the option parsing, qemu-options.hx, and all of >> that other junk. > glib's option parsing support has some really nice features like translation > support, grouping, and inline help for option definition. > The grouping is a really nice touch. It will let us have a target specific > group, device groups, etc. This sounds really nice and much more "uncrufty". What I'd like to see is some "type_init"-like black magic so that code can easily register GOptionEntry structures without having to resort to modifying a central file (otherwise I think the story will repeat itself again). For this to work, besides defining the GOptionEntry itself, the programmer should also establish: * The option group (for cmdline help grouping). This is already supported by GOptionGroup, and everybody should register against a set of pre-defined groups. * The parsing priority (so that it's easy to establish a partial order on the parsing of options). This also requires a set of pre-defined priorities to register against. What I'm not sure is whether both can be merged into a single one. That is, that options in the same group don't care about the order in which their parsing callbacks are called (as long as you know the order between groups). Lluis
diff --git a/vl.c b/vl.c index 0fccf50..fa4d0a9 100644 --- a/vl.c +++ b/vl.c @@ -2251,84 +2251,40 @@ int qemu_init_main_loop(void) return main_loop_init(); } -int main(int argc, char **argv, char **envp) -{ - int i; - int snapshot, linux_boot; - const char *icount_option = NULL; - const char *initrd_filename; - const char *kernel_filename, *kernel_cmdline; - char boot_devices[33] = "cad"; /* default to HD->floppy->CD-ROM */ - DisplayState *ds; - DisplayChangeListener *dcl; - int cyls, heads, secs, translation; - QemuOpts *hda_opts = NULL, *opts, *machine_opts; - QemuOptsList *olist; - int optind; - const char *optarg; - const char *loadvm = NULL; - QEMUMachine *machine; - const char *cpu_model; - const char *vga_model = NULL; - const char *pid_file = NULL; - const char *incoming = NULL; +int snapshot, linux_boot; +const char *icount_option; +const char *initrd_filename; +const char *kernel_filename, *kernel_cmdline; +char boot_devices[33] = "cad"; /* default to HD->floppy->CD-ROM */ +DisplayState *ds; +DisplayChangeListener *dcl; +int cyls, heads, secs, translation; +QemuOpts *hda_opts , *opts, *machine_opts; +QemuOptsList *olist; +int optind; +const char *loadvm; +QEMUMachine *machine; +const char *cpu_model; +const char *vga_model; +const char *pid_file; +const char *incoming; #ifdef CONFIG_VNC - int show_vnc_port = 0; +int show_vnc_port; #endif - int defconfig = 1; - const char *log_mask = NULL; - const char *log_file = NULL; - GMemVTable mem_trace = { - .malloc = malloc_and_trace, - .realloc = realloc_and_trace, - .free = free_and_trace, - }; - const char *trace_events = NULL; - const char *trace_file = NULL; - - atexit(qemu_run_exit_notifiers); - error_set_progname(argv[0]); - - g_mem_set_vtable(&mem_trace); - if (!g_thread_supported()) { -#if !GLIB_CHECK_VERSION(2, 31, 0) - g_thread_init(NULL); -#else - fprintf(stderr, "glib threading failed to initialize.\n"); - exit(1); -#endif - } - - module_call_init(MODULE_INIT_QOM); - - runstate_init(); - - init_clocks(); - rtc_clock = host_clock; - - qemu_cache_utils_init(envp); - - QLIST_INIT (&vm_change_state_head); - os_setup_early_signal_handling(); - - module_call_init(MODULE_INIT_MACHINE); - machine = find_default_machine(); - cpu_model = NULL; - ram_size = 0; - snapshot = 0; - cyls = heads = secs = 0; - translation = BIOS_ATA_TRANSLATION_AUTO; - - for (i = 0; i < MAX_NODES; i++) { - node_mem[i] = 0; - node_cpumask[i] = 0; - } - - nb_numa_nodes = 0; - nb_nics = 0; - - autostart= 1; +int defconfig = 1; +const char *log_mask; +const char *log_file; +GMemVTable mem_trace = { + .malloc = malloc_and_trace, + .realloc = realloc_and_trace, + .free = free_and_trace, +}; +const char *trace_events; +const char *trace_file; +static void options_parse(int argc, char **argv) +{ + const char *optarg; /* first pass of option parsing */ optind = 1; while (optind < argc) { @@ -2867,7 +2823,7 @@ int main(int argc, char **argv, char **envp) if (watchdog) { fprintf(stderr, "qemu: only one watchdog option may be given\n"); - return 1; + exit(1); } watchdog = optarg; break; @@ -3186,6 +3142,57 @@ int main(int argc, char **argv, char **envp) } } } +} + +int main(int argc, char **argv, char **envp) +{ + int i; + + atexit(qemu_run_exit_notifiers); + error_set_progname(argv[0]); + + g_mem_set_vtable(&mem_trace); + if (!g_thread_supported()) { +#if !GLIB_CHECK_VERSION(2, 31, 0) + g_thread_init(NULL); +#else + fprintf(stderr, "glib threading failed to initialize.\n"); + exit(1); +#endif + } + + module_call_init(MODULE_INIT_QOM); + + runstate_init(); + + init_clocks(); + rtc_clock = host_clock; + + qemu_cache_utils_init(envp); + + QLIST_INIT(&vm_change_state_head); + os_setup_early_signal_handling(); + + module_call_init(MODULE_INIT_MACHINE); + machine = find_default_machine(); + cpu_model = NULL; + ram_size = 0; + snapshot = 0; + cyls = heads = secs = 0; + translation = BIOS_ATA_TRANSLATION_AUTO; + + for (i = 0; i < MAX_NODES; i++) { + node_mem[i] = 0; + node_cpumask[i] = 0; + } + + nb_numa_nodes = 0; + nb_nics = 0; + + autostart = 1; + + options_parse(argc, argv); + loc_set_none(); /* Init CPU def lists, based on config
Consider of the options parse process in main function of vl.c is too long.It should be module into single function to clear ideas, strengthen the source code management, and increase code readability.So I module the process of options parse as function options_parse, and expose some variables in order to not influence command-line invocations. Signed-off-by: Wanpeng Li <liwp@linux.vnet.ibm.com> --- vl.c | 159 ++++++++++++++++++++++++++++++++++------------------------------- 1 files changed, 83 insertions(+), 76 deletions(-)