[Spice-devel] [PATCH v4 x11spice 2/3] Simplify the expression of option handling.

[Jeremy White]

This fixes a bug with --config=<filename> handling.

Add expanded test code courtesy of Frediano Ziglio <fziglio at redhat.com>.

Signed-off-by: Jeremy White <jwhite at codeweavers.com>
---
 src/main.c    |  8 +++--
 src/options.c | 93 ++++++++++++++++++++++++++++++++++++++++++---------
 src/options.h |  1 -
 3 files changed, 83 insertions(+), 19 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);
+    }
     if (rc)
         goto exit;
 
diff --git a/src/options.c b/src/options.c
index bc7cd631..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) {
-            str_replace(&options->user_config_file, 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) {
@@ -287,7 +278,7 @@ int options_parse_arguments(int argc, char *argv[], options_t *options)
                 break;
 
             case OPTION_CONFIG:
-                /* This was handled previously; we can ignore */
+                str_replace(&options->user_config_file, optarg);
                 break;
 
             case OPTION_SSL:
@@ -501,15 +492,85 @@ int options_impossible_config(options_t *options)
     return 1;
 }
 
-#if defined(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);
+    }
+    if (rc == 0) {
+        rc = options_process_io(options);
+    }
+    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);
-- 
2.20.1