Re: [Qemu-devel] [PATCH 06/12] qga: move option parsing to seperate function

[Michael Roth <mdroth@linux.vnet.ibm.com>]

Quoting Marc-André Lureau (2015-07-01 06:47:41)
> Move option parsing out of giant main().
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@gmail.com>
> ---
>  qga/main.c | 41 +++++++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/qga/main.c b/qga/main.c
> index b776d16..b965f61 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -941,19 +941,25 @@ static GList *split_list(gchar *str, const gchar separator)
>      return list;
>  }
> 
> -int main(int argc, char **argv)
> -{
> -    const char *sopt = "hVvdm:p:l:f:F::b:s:t:";
> -    char *method = NULL, *device_path = NULL;
> -    char *log_filepath = NULL;
> -    char *pid_filepath = NULL;
> +static char *device_path;
> +static char *method;
> +static char *log_filepath;
> +static char *pid_filepath;

Since we want to pass these around as a representation of the
configuration state, I'd rather we package them into a GAConfig
structure or something of the sort that and pass it around as arguments
rather than as globals. Between parse/load_config/load_defaults it's
becoming a little difficult to keep track of where all these values are
being modified.

Otherwise, looks good, and makes for a nice cleanup.

>  #ifdef CONFIG_FSFREEZE
> -    char *fsfreeze_hook = NULL;
> +static char *fsfreeze_hook;
>  #endif
> -    char *state_dir = NULL;
> +static char *state_dir;
>  #ifdef _WIN32
> -    const char *service = NULL;
> +static const char *service;
>  #endif
> +static GList *blacklist;
> +static int daemonize;
> +static GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
> +
> +static void option_parse(int argc, char **argv)
> +{
> +    const char *sopt = "hVvdm:p:l:f:F::b:s:t:D";
> +    int opt_ind = 0, ch;
>      const struct option lopt[] = {
>          { "help", 0, NULL, 'h' },
>          { "version", 0, NULL, 'V' },
> @@ -973,14 +979,7 @@ int main(int argc, char **argv)
>          { "statedir", 1, NULL, 't' },
>          { NULL, 0, NULL, 0 }
>      };
> -    int opt_ind = 0, ch, daemonize = 0;
> -    GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
> -    GList *blacklist = NULL;
> -    GAState *s;
> 
> -    module_call_init(MODULE_INIT_QAPI);
> -
> -    init_dfl_pathnames();
>      while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
>          switch (ch) {
>          case 'm':
> @@ -1058,6 +1057,16 @@ int main(int argc, char **argv)
>              exit(EXIT_FAILURE);
>          }
>      }
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    GAState *s;
> +
> +    module_call_init(MODULE_INIT_QAPI);
> +
> +    init_dfl_pathnames();
> +    option_parse(argc, argv);
> 
>      if (pid_filepath == NULL) {
>          pid_filepath = g_strdup(dfl_pathnames.pidfile);
> -- 
> 2.4.3
>