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

[Jeremy White]

On 7/23/19 11:27 AM, Frediano Ziglio wrote:
>>
>> 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?

The desired behavior is that the command line options override anything 
provided by the configuration files.

If a configuration file is provided on the command line, then we process 
only that file.  If no configuration file is given, then a config file 
in the users home directory overrides a config file in the system location.

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

That's fundamentally what my code does, with a stop to free data from 
the arguments along the way.  The str_replace semantic would remove the 
need for that extra step.

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

I had a look.  The str_replace semantic is a clear improvement, but is 
largely separate from this patch set.  You also caught an extraneous 
NULL check I missed :-/.

Your code, as written, does not handle the default configuration files.

To do that, you'd essentially have to trigger the 'again' stanza later. 
The functional result of the code would be identical to what I wrote. 
And, to be honest, I don't find that any more clear than the expression 
I submitted in this patch.

I'll use your str_replace semantic and revise this once again and see if 
it makes sense to you then.

Cheers,

Jeremy

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