>
> This fixes a bug with --config=handling.
>
> This also includes test code courtesy of Frediano Ziglio
> <fziglio@xxxxxxxxxx>.
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@xxxxxxxxxxxxxxx>
> ---
> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel