Re: [Qemu-devel] [PATCH] RFC: options parse in vl.c should be moduled

[Michael Roth]

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 <address@hidden>
> >> ---
> >>  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
> 
>