[Spice-devel] [PATCH v2 x11spice 3/4] Bug fix: support --config=<filename> semantics.

[Frediano Ziglio]

> 
> Instead of trying to parse argv ourselves, just reuse getopt_long_only.
> 
> Signed-off-by: Jeremy White <jwhite at codeweavers.com>

This patch looks a bit long compared to the job is doing.
What is the expected behaviour mixing --config and other options?
It looks like the configuration file should be always read first, all
other options, before or after --config should override the configuration
file. Is it right?

Why don't you call options_parse_arguments and if you find the
configuration file you then call options_handle_user_config and
options_parse_arguments again to just override the options loaded
from the file?

Was testing something like

https://cgit.freedesktop.org/~fziglio/x11spice/commit/?h=options&id=3595c4f06c0df5056e4cb65367a1de72b0c66056
with
https://cgit.freedesktop.org/~fziglio/x11spice/commit/?h=options&id=ef824f30a1a91ee77f44be7d86861535f7323b0a
(on top of your patches)

> ---
>  src/main.c    |  8 +++++---
>  src/options.c | 50 ++++++++++++++++++++++++++++++++++++--------------
>  src/options.h |  2 +-
>  3 files changed, 42 insertions(+), 18 deletions(-)
> 
> diff --git a/src/main.c b/src/main.c
> index f18311c9..72aba5a8 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -67,9 +67,11 @@ 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);
> +    rc = options_handle_user_config(argc, argv, &session.options);
> +    if (rc == 0) {
> +        options_from_config(&session.options);
> +        rc = options_parse_arguments(argc, argv, &session.options);
> +    }
>      if (rc)
>          goto exit;
>  
> diff --git a/src/options.c b/src/options.c
> index 303c07de..5a55f2c9 100644
> --- a/src/options.c
> +++ b/src/options.c
> @@ -224,16 +224,6 @@ void options_handle_ssl_file_options(options_t *options,
>      options->ssl.ciphersuite = string_option(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)
>  {
>      int rc;
> @@ -264,6 +254,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) {
> @@ -297,7 +288,9 @@ int options_parse_arguments(int argc, char *argv[],
> options_t *options)
>                  break;
>  
>              case OPTION_CONFIG:
> -                /* This was handled previously; we can ignore */
> +                if (options->user_config_file)
> +                    g_free(options->user_config_file);
> +                options->user_config_file = g_strdup(optarg);
>                  break;
>  
>              case OPTION_SSL:
> @@ -358,6 +351,21 @@ int options_parse_arguments(int argc, char *argv[],
> options_t *options)
>      return rc;
>  }
>  
> +/* The idea is to have command line arguments run after all other
> +**  configuration, with one exception - we want to grab any
> +**  user specified configuration file first. */
> +int options_handle_user_config(int argc, char *argv[], options_t *options)
> +{
> +    options_t tmp_options = { 0 };
> +    int rc;
> +
> +    rc = options_parse_arguments(argc, argv, &tmp_options);
> +    options->user_config_file = tmp_options.user_config_file;
> +    tmp_options.user_config_file = NULL;
> +    options_free(&tmp_options);
> +    return rc;
> +}
> +
>  void options_from_config(options_t *options)
>  {
>      GKeyFile *userkey = g_key_file_new();
> @@ -517,11 +525,25 @@ int options_impossible_config(options_t *options)
>  int main(int argc, char *argv[])
>  {
>      options_t options;
> +    int rc;
>  
>      options_init(&options);
> -    options_parse_arguments(argc, argv, &options);
> -    options_from_config(&options);
> -    g_message("Options parsed");
> +    rc = options_handle_user_config(argc, argv, &options);
> +    if (rc == 0) {
> +        options_from_config(&options);
> +        rc = options_parse_arguments(argc, argv, &options);
> +    }
> +    if (rc == 0) {
> +        g_message("Options parsed");
> +        rc = options_process_io(&options);
> +        if (rc == 0)
> +            g_message("Options io run parsed");
> +        else
> +            g_message("Options io failed");
> +    }
> +    else {
> +        g_message("Options parse failed");
> +    }
>      options_free(&options);
>  }
>  #endif
> diff --git a/src/options.h b/src/options.h
> index 6155984b..162643ac 100644
> --- a/src/options.h
> +++ b/src/options.h
> @@ -73,7 +73,7 @@ typedef struct {
>  **  Prototypes
>  **--------------------------------------------------------------------------*/
>  void options_init(options_t *options);
> -void options_handle_user_config(int argc, char *argv[], options_t *options);
> +int 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);

Frediano