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

[Frediano Ziglio]

> 
> This fixes a bug with --config=handling.
> 
> This also includes test code courtesy of Frediano Ziglio
> <fziglio at redhat.com>.

This was very experiment patch, just worked.
I would turn it in a real test as there's a tests directory.
It seems is using just exported APIs so maybe would be better
to put a new test (executable) in tests directory having a
main function and calling/linking options.c.

> 
> Signed-off-by: Jeremy White <jwhite at codeweavers.com>
> ---
>  src/main.c    |  8 +++--
>  src/options.c | 91 ++++++++++++++++++++++++++++++++++++++++++---------
>  src/options.h |  1 -
>  3 files changed, 82 insertions(+), 18 deletions(-)
> 
> diff --git a/src/main.c b/src/main.c
> index f18311c9..da6d4882 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -67,9 +67,13 @@ 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 == 0) {
> +        options_from_config(&session.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, &session.options);
> +    }

I personally find a bit complicated API to use, a simple call to
a single function would be IMHO better.

>      if (rc)
>          goto exit;
>  
> diff --git a/src/options.c b/src/options.c
> index cff4ac17..6620b853 100644
> --- a/src/options.c
> +++ b/src/options.c
> @@ -214,16 +214,6 @@ 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)
>  {
>      int rc;
> @@ -254,6 +244,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) {
> @@ -501,15 +492,85 @@ int options_impossible_config(options_t *options)
>      return 1;
>  }
>  
> -#if defined(OPTIONS_MAIN)

What was this OPTIONS_MAIN ?

> -int main(int argc, char *argv[])
> +#if defined(MAIN_TEST)
> +static int test_load_options(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);
> +    }

quite boring that every time you need to use options_parse_arguments
you have to repeat these step.

> +    if (rc == 0) {
> +        rc = options_process_io(options);

Is this always mandatory? Can the user of these API avoid to call it?

> +    }
> +    return rc;
> +}
> +
> +static void test(const char *expected, char **argv)
>  {
>      options_t options;
> +    int argc = 0;
> +    int rc;
> +    char buf[1000];
> +
> +    while (argv[argc]) {
> +        ++argc;
> +    }
>  
>      options_init(&options);
> -    options_parse_arguments(argc, argv, &options);
> -    options_from_config(&options);
> -    g_message("Options parsed");
> +    rc = test_load_options(&options, argc, argv);
> +    if (rc == 0) {
> +        snprintf(buf, sizeof(buf), "display %s allow_control %d listen %s
> ca_file %s",
> +                 options.display, options.allow_control, options.listen,
> options.ssl.ca_cert_file);
> +        if (strcmp(buf, expected) != 0) {
> +            fprintf(stderr, "Unexpected results:\n\t%s\n%s\n",
> +                    buf, expected);
> +        }
> +    }
> +    else {
> +        fprintf(stderr, "Unable to load options, rc %d\n", rc);
> +    }
>      options_free(&options);
>  }
> +
> +#define TEST(exp, ...) do { \
> +    char *argv[] = { "PROGRAM", __VA_ARGS__, NULL }; \
> +    test(exp, argv); \
> +} while(0)
> +
> +int main(int argc, char **argv)
> +{
> +    FILE *f = fopen("test.conf", "w");
> +
> +    if (argc > 1) {
> +        options_t options;
> +        options_init(&options);
> +        printf("test_load_options returns %d\n",
> +            test_load_options(&options, argc, argv));
> +        options_free(&options);
> +    }
> +
> +    fprintf(f,
> "[spice]\ndisplay=config_display\nlisten=8765\nallow-control=true");
> +    fclose(f);
> +
> +    TEST("display (null) allow_control 0 listen 5900 ca_file (null)",
> +         "--hide");
> +    TEST("display (null) allow_control 0 listen 1234 ca_file (null)",
> +         "1234");
> +    TEST("display config_display allow_control 1 listen 8765 ca_file
> (null)",
> +         "--config=test.conf");
> +    TEST("display config_display allow_control 1 listen 123 ca_file (null)",
> +         "--config=test.conf", "123");
> +    TEST("display config_display allow_control 0 listen 123 ca_file (null)",
> +         "--no-allow-control", "--config=test.conf", "123");
> +    TEST("display DISPLAY allow_control 1 listen 123 ca_file (null)",
> +         "--display", "DISPLAY", "--config=test.conf", "123");
> +    TEST("display (null) allow_control 0 listen 5900 ca_file foo",
> +         "--ssl=foo");
> +    unlink("test.conf");
> +    return 0;
> +}
>  #endif
> diff --git a/src/options.h b/src/options.h
> index 6155984b..2302c85b 100644
> --- a/src/options.h
> +++ b/src/options.h
> @@ -73,7 +73,6 @@ 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);

Frediano