Re: [PATCH v2 21/22] qga: allow configuration file path via the cli

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 13/6/24 17:44, Daniel P. Berrangé wrote:
> Allowing the user to set the QGA_CONF environment variable to change
> the default configuration file path is very unusual practice, made
> more obscure since this ability is not documented.
> 
> This introduces the more normal '-c PATH'  / '--config=PATH' command
> line argument approach. This requires that we parse the comamnd line
> twice, since we want the command line arguments to take priority over
> the configuration file settings in general.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   docs/interop/qemu-ga.rst |  5 +++++
>   qga/main.c               | 35 +++++++++++++++++++++++++++--------
>   2 files changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/docs/interop/qemu-ga.rst b/docs/interop/qemu-ga.rst
> index 72fb75a6f5..e42b370319 100644
> --- a/docs/interop/qemu-ga.rst
> +++ b/docs/interop/qemu-ga.rst
> @@ -33,6 +33,11 @@ Options
>   
>   .. program:: qemu-ga
>   
> +.. option:: -c, --config=PATH
> +
> +  Configuration file path (the default is |CONFDIR|\ ``/qemu-ga.conf``,
> +  unless overriden by the QGA_CONF environment variable)
> +
>   .. option:: -m, --method=METHOD
>   
>     Transport method: one of ``unix-listen``, ``virtio-serial``, or
> diff --git a/qga/main.c b/qga/main.c
> index 6ff022a85d..f68a32bf7b 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -1018,15 +1018,14 @@ static GList *split_list(const gchar *str, const gchar *delim)
>       return list;
>   }
>   
> -static void config_load(GAConfig *config)
> +static void config_load(GAConfig *config, const char *confpath, bool required)
>   {
>       GError *gerr = NULL;
>       GKeyFile *keyfile;
> -    g_autofree char *conf = g_strdup(g_getenv("QGA_CONF")) ?: get_relocated_path(QGA_CONF_DEFAULT);
>   
>       /* read system config */
>       keyfile = g_key_file_new();
> -    if (!g_key_file_load_from_file(keyfile, conf, 0, &gerr)) {
> +    if (!g_key_file_load_from_file(keyfile, confpath, 0, &gerr)) {
>           goto end;
>       }
>       if (g_key_file_has_key(keyfile, "general", "daemon", NULL)) {
> @@ -1092,10 +1091,10 @@ static void config_load(GAConfig *config)
>   
>   end:
>       g_key_file_free(keyfile);
> -    if (gerr &&
> -        !(gerr->domain == G_FILE_ERROR && gerr->code == G_FILE_ERROR_NOENT)) {
> +    if (gerr && (required ||
> +                 !(gerr->domain == G_FILE_ERROR && gerr->code == G_FILE_ERROR_NOENT))) {
>           g_critical("error loading configuration from path: %s, %s",
> -                   conf, gerr->message);
> +                   confpath, gerr->message);
>           exit(EXIT_FAILURE);
>       }
>       g_clear_error(&gerr);
> @@ -1167,12 +1166,13 @@ static void config_dump(GAConfig *config)
>   
>   static void config_parse(GAConfig *config, int argc, char **argv)
>   {
> -    const char *sopt = "hVvdm:p:l:f:F::b:a:s:t:Dr";
> +    const char *sopt = "hVvdc:m:p:l:f:F::b:a:s:t:Dr";
>       int opt_ind = 0, ch;
>       bool block_rpcs = false, allow_rpcs = false;
>       const struct option lopt[] = {
>           { "help", 0, NULL, 'h' },
>           { "version", 0, NULL, 'V' },
> +        { "config", 1, NULL, 'c' },
>           { "dump-conf", 0, NULL, 'D' },
>           { "logfile", 1, NULL, 'l' },
>           { "pidfile", 1, NULL, 'f' },
> @@ -1192,6 +1192,26 @@ static void config_parse(GAConfig *config, int argc, char **argv)
>           { "retry-path", 0, NULL, 'r' },
>           { NULL, 0, NULL, 0 }
>       };
> +    g_autofree char *confpath = g_strdup(g_getenv("QGA_CONF")) ?:
> +        get_relocated_path(QGA_CONF_DEFAULT);
> +    bool confrequired = false;
> +
> +    while ((ch = getopt_long(argc, argv, sopt, lopt, NULL)) != -1) {
> +        switch (ch) {
> +        case 'c':
> +            g_free(confpath);
> +            confpath = g_strdup(optarg);
> +            confrequired = true;
> +            break;
> +        default:
> +            break;
> +        }
> +    }
> +
> +    config_load(config, confpath, confrequired);
> +
> +    /* Reset for second pass */
> +    optind = 1;
>   
>       while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
>           switch (ch) {
> @@ -1582,7 +1602,6 @@ int main(int argc, char **argv)
>       qga_qmp_init_marshal(&ga_commands);
>   
>       init_dfl_pathnames();
> -    config_load(config);
>       config_parse(config, argc, argv);
>   
>       if (config->pid_filepath == NULL) {

This looks like a trivial (correct) CLI change, but
I don't feel confident anymore reviewing this area,
so will let others have a look.

Regards,

Phil.