[Spice-devel] [PATCH v2 fixup1 x11spice 2/3] Simplify the expression of argument parsing.

[Frediano Ziglio]

> 
> This fixes a bug with --config=handling.
> 
> Signed-off-by: Jeremy White <jwhite at codeweavers.com>
> ---
> v2: Simplify even further.
> ---
>  src/main.c    |  8 +----
>  src/options.c | 96 ++++++++++++++++++++++++---------------------------
>  src/options.h |  5 +--
>  3 files changed, 47 insertions(+), 62 deletions(-)
> 
> diff --git a/src/main.c b/src/main.c
> index f18311c9..cecadc8d 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -67,13 +67,7 @@ int main(int argc, char *argv[])
>      **  Parse arguments
>      **----------------------------------------------------------------------*/
>      options_init(&session.options);
> -    options_handle_user_config(argc, argv, &session.options);
> -    options_from_config(&session.options);
> -    rc = options_parse_arguments(argc, argv, &session.options);
> -    if (rc)
> -        goto exit;
> -
> -    rc = options_process_io(&session.options);
> +    rc = options_load(&session.options, argc, argv);
>      if (rc)
>          goto exit;
>  
> diff --git a/src/options.c b/src/options.c
> index cff4ac17..3a59d964 100644
> --- a/src/options.c
> +++ b/src/options.c
> @@ -51,11 +51,6 @@ static void str_replace(char **dest, const char *src)
>      *dest = src ? g_strdup(src) : NULL;
>  }
>  
> -void options_init(options_t *options)
> -{
> -    memset(options, 0, sizeof(*options));
> -}
> -
>  static void ssl_options_free(ssl_options_t *ssl)
>  {
>      str_replace(&ssl->ca_cert_file, NULL);
> @@ -66,24 +61,6 @@ static void ssl_options_free(ssl_options_t *ssl)
>      str_replace(&ssl->ciphersuite, NULL);
>  }
>  
> -void options_free(options_t *options)
> -{
> -    str_replace(&options->display, NULL);
> -    str_replace(&options->listen, NULL);
> -
> -    ssl_options_free(&options->ssl);
> -    str_replace(&options->spice_password, NULL);
> -    str_replace(&options->password_file, NULL);
> -
> -    str_replace(&options->virtio_path, NULL);
> -    str_replace(&options->uinput_path, NULL);
> -    str_replace(&options->on_connect, NULL);
> -    str_replace(&options->on_disconnect, NULL);
> -    str_replace(&options->user_config_file, NULL);
> -    str_replace(&options->system_config_file, NULL);
> -}
> -
> -

Why did you move these function?
Looks like useless and make the commit bigger.
Also there's no mention on the commit message.
If there's a reason I would put the move in a separate commit.

>  static void string_option(gchar **dest, GKeyFile *u, GKeyFile *s, const
>  gchar *section, const gchar *key)
>  {
>      gchar *ret = NULL;
> @@ -160,7 +137,7 @@ static void usage(char *argv0)
>      printf("%s [--minimize]\n", indent);
>  }
>  
> -int options_handle_ssl(options_t *options, const char *spec)
> +static int options_handle_ssl(options_t *options, const char *spec)
>  {
>      char *save = NULL;
>      char *in = g_strdup(spec);
> @@ -202,7 +179,7 @@ int options_handle_ssl(options_t *options, const char
> *spec)
>      return rc;
>  }
>  
> -void options_handle_ssl_file_options(options_t *options,
> +static void options_handle_ssl_file_options(options_t *options,
>                                       GKeyFile *userkey, GKeyFile *systemkey)
>  {
>      options->ssl.enabled = bool_option(userkey, systemkey, "ssl",
>      "enabled");
> @@ -214,17 +191,7 @@ void options_handle_ssl_file_options(options_t *options,
>      string_option(&options->ssl.ciphersuite, userkey, systemkey, "ssl",
>      "ciphersuite");
>  }
>  
> -void options_handle_user_config(int argc, char *argv[], options_t *options)
> -{
> -    int i;
> -    for (i = 1; i < argc - 1; i++)
> -        if (strcmp(argv[i], "--config") == 0 || strcmp(argv[i], "-config")
> == 0) {
> -            options->user_config_file = g_strdup(argv[i + 1]);
> -            i++;
> -        }
> -}
> -
> -int options_parse_arguments(int argc, char *argv[], options_t *options)
> +static int options_parse_arguments(int argc, char *argv[], options_t
> *options)
>  {
>      int rc;
>      int longindex = 0;
> @@ -254,6 +221,7 @@ int options_parse_arguments(int argc, char *argv[],
> options_t *options)
>          {0, 0, 0, 0}
>      };
>  
> +    optind = 1; /* Allow reuse of this function */
>      while (1) {
>          rc = getopt_long_only(argc, argv, "", long_options, &longindex);
>          if (rc == -1) {
> @@ -348,7 +316,7 @@ int options_parse_arguments(int argc, char *argv[],
> options_t *options)
>      return rc;
>  }
>  
> -void options_from_config(options_t *options)
> +static void options_from_config(options_t *options)
>  {
>      GKeyFile *userkey = g_key_file_new();
>      GKeyFile *systemkey = NULL;
> @@ -467,7 +435,7 @@ static int generate_password(options_t *options)
>      return 0;
>  }
>  
> -int options_process_io(options_t *options)
> +static int options_process_io(options_t *options)
>  {
>      int rc;
>      if (options->password_file) {
> @@ -487,6 +455,45 @@ int options_process_io(options_t *options)
>      return 0;
>  }
>  
> +void options_init(options_t *options)
> +{
> +    memset(options, 0, sizeof(*options));
> +}
> +
> +void options_free(options_t *options)
> +{
> +    str_replace(&options->display, NULL);
> +    str_replace(&options->listen, NULL);
> +
> +    ssl_options_free(&options->ssl);
> +    str_replace(&options->spice_password, NULL);
> +    str_replace(&options->password_file, NULL);
> +
> +    str_replace(&options->virtio_path, NULL);
> +    str_replace(&options->uinput_path, NULL);
> +    str_replace(&options->on_connect, NULL);
> +    str_replace(&options->on_disconnect, NULL);
> +    str_replace(&options->user_config_file, NULL);
> +    str_replace(&options->system_config_file, NULL);
> +}
> +
> +int options_load(options_t *options, int argc, char *argv[])
> +{
> +    int rc;
> +
> +    rc = options_parse_arguments(argc, argv, options);
> +    if (rc == 0) {
> +        options_from_config(options);
> +        /* We parse command line arguments a second time to ensure
> +        **  that command line options take precedence over config files */
> +        rc = options_parse_arguments(argc, argv, options);
> +        if (rc == 0)
> +            rc = options_process_io(options);
> +    }
> +    return rc;
> +}
> +
> +
>  int options_impossible_config(options_t *options)
>  {
>      if (options->spice_password)
> @@ -500,16 +507,3 @@ int options_impossible_config(options_t *options)
>  
>      return 1;
>  }
> -
> -#if defined(OPTIONS_MAIN)
> -int main(int argc, char *argv[])
> -{
> -    options_t options;
> -
> -    options_init(&options);
> -    options_parse_arguments(argc, argv, &options);
> -    options_from_config(&options);
> -    g_message("Options parsed");
> -    options_free(&options);
> -}
> -#endif

I would mention in the commit message, something along:

"Removed old testing code as obsolete instead of updating unused code."


> diff --git a/src/options.h b/src/options.h
> index 6155984b..e7cdece1 100644
> --- a/src/options.h
> +++ b/src/options.h
> @@ -73,11 +73,8 @@ typedef struct {
>  **  Prototypes
>  **--------------------------------------------------------------------------*/
>  void options_init(options_t *options);
> -void options_handle_user_config(int argc, char *argv[], options_t *options);
> -int options_parse_arguments(int argc, char *argv[], options_t *options);
> -int options_process_io(options_t *options);
>  void options_free(options_t *options);
> -void options_from_config(options_t *options);
> +int options_load(options_t *options, int argc, char *argv[]);
>  int options_impossible_config(options_t *options);
>  
>  #endif

Frediano