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

[Jeremy White]

On 7/24/19 9:59 AM, Frediano Ziglio wrote:
>>
>> 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.

That feels like serious overkill to me.  This code is essentially a 
utility to the author to give a hook to run valgrind against, it's not 
intended as production test code.  Perhaps I'll strip it out to avoid 
confusion, and just leave it in a private branch.

> 
>>
>> 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.

Sure.

> 
>>       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 ?

That was the previous test code.

> 
>> -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?

The point of this was to exercise as much of the code path as possible, 
as an aid to running it under valgrind.

> 
>> +    }
>> +    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
>