diff mbox series

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

Message ID 20240613154406.1365469-16-berrange@redhat.com (mailing list archive)
State New, archived
Headers show
Series qga: clean up command source locations and conditionals | expand

Commit Message

Daniel P. Berrangé June 13, 2024, 3:44 p.m. UTC 
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(-)

Comments

Philippe Mathieu-Daudé July 3, 2024, 8:44 a.m. UTC | #1 
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.
Konstantin Kostiuk July 12, 2024, 9:05 a.m. UTC | #2 
On Thu, Jun 13, 2024 at 6:45 PM Daniel P. Berrangé <berrange@redhat.com>
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
>

Please also update qga/main.c static void usage(const char *cmd)


>
>    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) {
> --
> 2.45.1
>
>
Daniel P. Berrangé July 12, 2024, 9:18 a.m. UTC | #3 
On Fri, Jul 12, 2024 at 12:05:23PM +0300, Konstantin Kostiuk wrote:
> On Thu, Jun 13, 2024 at 6:45 PM Daniel P. Berrangé <berrange@redhat.com>
> 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
> >
> 
> Please also update qga/main.c static void usage(const char *cmd)

Opps, yes, will do.


With regards,
Daniel
diff mbox series

Patch

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