diff mbox

[v2,10/12] qga: add an optionnal qemu-ga.conf system configuration

Message ID 1440540624-7998-11-git-send-email-marcandre.lureau@redhat.com
State New
Headers show

Commit Message

Marc-André Lureau Aug. 25, 2015, 10:10 p.m. UTC 
From: Marc-André Lureau <marcandre.lureau@redhat.com>

Learn to configure the agent with a system configuration.

This may simplify command-line handling, especially when the blacklist
is long.

Among the other benefits, this may standardize the configuration of a
init service (instead of distro-specific init keys/files)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qga/main.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 73 insertions(+), 7 deletions(-)

Comments

Michael Roth Aug. 25, 2015, 11:09 p.m. UTC | #1 
Quoting marcandre.lureau@redhat.com (2015-08-25 17:10:22)
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Learn to configure the agent with a system configuration.
> 
> This may simplify command-line handling, especially when the blacklist
> is long.
> 
> Among the other benefits, this may standardize the configuration of a
> init service (instead of distro-specific init keys/files)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qga/main.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 73 insertions(+), 7 deletions(-)
> 
> diff --git a/qga/main.c b/qga/main.c
> index 58f2fc7..154975c 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -56,6 +56,7 @@
>  #define QGA_FSFREEZE_HOOK_DEFAULT CONFIG_QEMU_CONFDIR "/fsfreeze-hook"
>  #endif
>  #define QGA_SENTINEL_BYTE 0xFF
> +#define QGA_CONF_DEFAULT CONFIG_QEMU_CONFDIR G_DIR_SEPARATOR_S "qemu-ga.conf"

We should probably do this in init_dfl_pathnames() for consistency.

> 
>  static struct {
>      const char *state_dir;
> @@ -936,14 +937,80 @@ typedef struct GAConfig {
>  #ifdef _WIN32
>      const char *service;
>  #endif
> +    gchar *bliststr;
>      GList *blacklist;
>      int daemonize;
>      GLogLevelFlags log_level;
>  } GAConfig;
> 
> -static GAConfig *config_parse(int argc, char **argv)
> +static void config_load(GAConfig *config)
> +{
> +    GError *gerr = NULL;
> +    GKeyFile *keyfile;
> +
> +    /* read system config */
> +    keyfile = g_key_file_new();
> +    if (!g_key_file_load_from_file(keyfile, QGA_CONF_DEFAULT, 0, &gerr)) {
> +        goto end;
> +    }
> +    if (g_key_file_has_key(keyfile, "general", "daemon", NULL)) {
> +        config->daemonize =
> +            g_key_file_get_boolean(keyfile, "general", "daemon", &gerr);
> +    }
> +    if (g_key_file_has_key(keyfile, "general", "method", NULL)) {
> +        config->method =
> +            g_key_file_get_string(keyfile, "general", "method", &gerr);
> +    }
> +    if (g_key_file_has_key(keyfile, "general", "path", NULL)) {
> +        config->channel_path =
> +            g_key_file_get_string(keyfile, "general", "path", &gerr);
> +    }
> +    if (g_key_file_has_key(keyfile, "general", "logfile", NULL)) {
> +        config->log_filepath =
> +            g_key_file_get_string(keyfile, "general", "logfile", &gerr);
> +    }
> +    if (g_key_file_has_key(keyfile, "general", "pidfile", NULL)) {
> +        config->pid_filepath =
> +            g_key_file_get_string(keyfile, "general", "pidfile", &gerr);
> +    }
> +#ifdef CONFIG_FSFREEZE
> +    if (g_key_file_has_key(keyfile, "general", "fsfreeze-hook", NULL)) {
> +        config->fsfreeze_hook =
> +            g_key_file_get_string(keyfile,
> +                                  "general", "fsfreeze-hook", &gerr);
> +    }
> +#endif
> +    if (g_key_file_has_key(keyfile, "general", "statedir", NULL)) {
> +        config->state_dir =
> +            g_key_file_get_string(keyfile, "general", "statedir", &gerr);
> +    }
> +    if (g_key_file_has_key(keyfile, "general", "verbose", NULL) &&
> +        g_key_file_get_boolean(keyfile, "general", "verbose", &gerr)) {
> +        /* enable all log levels */
> +        config->log_level = G_LOG_LEVEL_MASK;
> +    }
> +    if (g_key_file_has_key(keyfile, "general", "blacklist", NULL)) {
> +        config->bliststr =
> +            g_key_file_get_string(keyfile, "general", "blacklist", &gerr);
> +        config->blacklist = g_list_concat(config->blacklist,
> +                                          split_list(config->bliststr, ','));
> +    }
> +
> +end:
> +    if (keyfile) {
> +        g_key_file_free(keyfile);
> +    }
> +    if (gerr &&
> +        !(gerr->domain == G_FILE_ERROR && gerr->code == G_FILE_ERROR_NOENT)) {
> +        g_critical("error loading configuration from path: %s, %s",
> +                   QGA_CONF_DEFAULT, gerr->message);
> +        exit(EXIT_FAILURE);
> +    }
> +    g_clear_error(&gerr);

What about other file errors, like permission issues? Maybe just have
config_load() return bool, and just spit out stringified gerr prior to
return false?

Looks good otherwise.

> +}
> +
> +static void config_parse(GAConfig *config, int argc, char **argv)
>  {
> -    GAConfig *config = g_new0(GAConfig, 1);
>      const char *sopt = "hVvdm:p:l:f:F::b:s:t:D";
>      int opt_ind = 0, ch;
>      const struct option lopt[] = {
> @@ -1047,8 +1114,6 @@ static GAConfig *config_parse(int argc, char **argv)
>              exit(EXIT_FAILURE);
>          }
>      }
> -
> -    return config;
>  }
> 
>  static void config_free(GAConfig *config)
> @@ -1058,6 +1123,7 @@ static void config_free(GAConfig *config)
>      g_free(config->pid_filepath);
>      g_free(config->state_dir);
>      g_free(config->channel_path);
> +    g_free(config->bliststr);
>  #ifdef CONFIG_FSFREEZE
>      g_free(config->fsfreeze_hook);
>  #endif
> @@ -1200,13 +1266,13 @@ int main(int argc, char **argv)
>  {
>      int ret = EXIT_SUCCESS;
>      GAState *s = g_new0(GAState, 1);
> -    GAConfig *config;
> +    GAConfig *config = g_new0(GAConfig, 1);
> 
>      module_call_init(MODULE_INIT_QAPI);
> 
>      init_dfl_pathnames();
> -
> -    config = config_parse(argc, argv);
> +    config_load(config);
> +    config_parse(config, argc, argv);
> 
>      if (config->pid_filepath == NULL) {
>          config->pid_filepath = g_strdup(dfl_pathnames.pidfile);
> -- 
> 2.4.3
>
Marc-André Lureau Aug. 25, 2015, 11:18 p.m. UTC | #2 
Hi

On Wed, Aug 26, 2015 at 1:09 AM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> We should probably do this in init_dfl_pathnames() for consistency.

QGA_FSFREEZE_HOOK_DEFAULT is also initialized here

in init_dfl_pathnames(), it depends on the value of
get_local_state_pathname() while here it's static.

>>
>>  static struct {
>>      const char *state_dir;
>> @@ -936,14 +937,80 @@ typedef struct GAConfig {
>>  #ifdef _WIN32
>>      const char *service;
>>  #endif
>> +    gchar *bliststr;
>>      GList *blacklist;
>>      int daemonize;
>>      GLogLevelFlags log_level;
>>  } GAConfig;
>>
>> -static GAConfig *config_parse(int argc, char **argv)
>> +static void config_load(GAConfig *config)
>> +{
>> +    GError *gerr = NULL;
>> +    GKeyFile *keyfile;
>> +
>> +    /* read system config */
>> +    keyfile = g_key_file_new();
>> +    if (!g_key_file_load_from_file(keyfile, QGA_CONF_DEFAULT, 0, &gerr)) {
>> +        goto end;
>> +    }
>> +    if (g_key_file_has_key(keyfile, "general", "daemon", NULL)) {
>> +        config->daemonize =
>> +            g_key_file_get_boolean(keyfile, "general", "daemon", &gerr);
>> +    }
>> +    if (g_key_file_has_key(keyfile, "general", "method", NULL)) {
>> +        config->method =
>> +            g_key_file_get_string(keyfile, "general", "method", &gerr);
>> +    }
>> +    if (g_key_file_has_key(keyfile, "general", "path", NULL)) {
>> +        config->channel_path =
>> +            g_key_file_get_string(keyfile, "general", "path", &gerr);
>> +    }
>> +    if (g_key_file_has_key(keyfile, "general", "logfile", NULL)) {
>> +        config->log_filepath =
>> +            g_key_file_get_string(keyfile, "general", "logfile", &gerr);
>> +    }
>> +    if (g_key_file_has_key(keyfile, "general", "pidfile", NULL)) {
>> +        config->pid_filepath =
>> +            g_key_file_get_string(keyfile, "general", "pidfile", &gerr);
>> +    }
>> +#ifdef CONFIG_FSFREEZE
>> +    if (g_key_file_has_key(keyfile, "general", "fsfreeze-hook", NULL)) {
>> +        config->fsfreeze_hook =
>> +            g_key_file_get_string(keyfile,
>> +                                  "general", "fsfreeze-hook", &gerr);
>> +    }
>> +#endif
>> +    if (g_key_file_has_key(keyfile, "general", "statedir", NULL)) {
>> +        config->state_dir =
>> +            g_key_file_get_string(keyfile, "general", "statedir", &gerr);
>> +    }
>> +    if (g_key_file_has_key(keyfile, "general", "verbose", NULL) &&
>> +        g_key_file_get_boolean(keyfile, "general", "verbose", &gerr)) {
>> +        /* enable all log levels */
>> +        config->log_level = G_LOG_LEVEL_MASK;
>> +    }
>> +    if (g_key_file_has_key(keyfile, "general", "blacklist", NULL)) {
>> +        config->bliststr =
>> +            g_key_file_get_string(keyfile, "general", "blacklist", &gerr);
>> +        config->blacklist = g_list_concat(config->blacklist,
>> +                                          split_list(config->bliststr, ','));
>> +    }
>> +
>> +end:
>> +    if (keyfile) {
>> +        g_key_file_free(keyfile);
>> +    }
>> +    if (gerr &&
>> +        !(gerr->domain == G_FILE_ERROR && gerr->code == G_FILE_ERROR_NOENT)) {
>> +        g_critical("error loading configuration from path: %s, %s",
>> +                   QGA_CONF_DEFAULT, gerr->message);
>> +        exit(EXIT_FAILURE);
>> +    }
>> +    g_clear_error(&gerr);
>
> What about other file errors, like permission issues? Maybe just have
> config_load() return bool, and just spit out stringified gerr prior to
> return false?

all errors have a g_critical(), except for the case of file missing.
Michael Roth Aug. 25, 2015, 11:29 p.m. UTC | #3 
Quoting Marc-André Lureau (2015-08-25 18:18:16)
> Hi
> 
> On Wed, Aug 26, 2015 at 1:09 AM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> > We should probably do this in init_dfl_pathnames() for consistency.
> 
> QGA_FSFREEZE_HOOK_DEFAULT is also initialized here
> 
> in init_dfl_pathnames(), it depends on the value of
> get_local_state_pathname() while here it's static.

Ahh, yah I missed that. It's fine where it's at then.

> 
> >>
> >>  static struct {
> >>      const char *state_dir;
> >> @@ -936,14 +937,80 @@ typedef struct GAConfig {
> >>  #ifdef _WIN32
> >>      const char *service;
> >>  #endif
> >> +    gchar *bliststr;
> >>      GList *blacklist;
> >>      int daemonize;
> >>      GLogLevelFlags log_level;
> >>  } GAConfig;
> >>
> >> -static GAConfig *config_parse(int argc, char **argv)
> >> +static void config_load(GAConfig *config)
> >> +{
> >> +    GError *gerr = NULL;
> >> +    GKeyFile *keyfile;
> >> +
> >> +    /* read system config */
> >> +    keyfile = g_key_file_new();
> >> +    if (!g_key_file_load_from_file(keyfile, QGA_CONF_DEFAULT, 0, &gerr)) {
> >> +        goto end;
> >> +    }
> >> +    if (g_key_file_has_key(keyfile, "general", "daemon", NULL)) {
> >> +        config->daemonize =
> >> +            g_key_file_get_boolean(keyfile, "general", "daemon", &gerr);
> >> +    }
> >> +    if (g_key_file_has_key(keyfile, "general", "method", NULL)) {
> >> +        config->method =
> >> +            g_key_file_get_string(keyfile, "general", "method", &gerr);
> >> +    }
> >> +    if (g_key_file_has_key(keyfile, "general", "path", NULL)) {
> >> +        config->channel_path =
> >> +            g_key_file_get_string(keyfile, "general", "path", &gerr);
> >> +    }
> >> +    if (g_key_file_has_key(keyfile, "general", "logfile", NULL)) {
> >> +        config->log_filepath =
> >> +            g_key_file_get_string(keyfile, "general", "logfile", &gerr);
> >> +    }
> >> +    if (g_key_file_has_key(keyfile, "general", "pidfile", NULL)) {
> >> +        config->pid_filepath =
> >> +            g_key_file_get_string(keyfile, "general", "pidfile", &gerr);
> >> +    }
> >> +#ifdef CONFIG_FSFREEZE
> >> +    if (g_key_file_has_key(keyfile, "general", "fsfreeze-hook", NULL)) {
> >> +        config->fsfreeze_hook =
> >> +            g_key_file_get_string(keyfile,
> >> +                                  "general", "fsfreeze-hook", &gerr);
> >> +    }
> >> +#endif
> >> +    if (g_key_file_has_key(keyfile, "general", "statedir", NULL)) {
> >> +        config->state_dir =
> >> +            g_key_file_get_string(keyfile, "general", "statedir", &gerr);
> >> +    }
> >> +    if (g_key_file_has_key(keyfile, "general", "verbose", NULL) &&
> >> +        g_key_file_get_boolean(keyfile, "general", "verbose", &gerr)) {
> >> +        /* enable all log levels */
> >> +        config->log_level = G_LOG_LEVEL_MASK;
> >> +    }
> >> +    if (g_key_file_has_key(keyfile, "general", "blacklist", NULL)) {
> >> +        config->bliststr =
> >> +            g_key_file_get_string(keyfile, "general", "blacklist", &gerr);
> >> +        config->blacklist = g_list_concat(config->blacklist,
> >> +                                          split_list(config->bliststr, ','));
> >> +    }
> >> +
> >> +end:
> >> +    if (keyfile) {
> >> +        g_key_file_free(keyfile);
> >> +    }
> >> +    if (gerr &&
> >> +        !(gerr->domain == G_FILE_ERROR && gerr->code == G_FILE_ERROR_NOENT)) {
> >> +        g_critical("error loading configuration from path: %s, %s",
> >> +                   QGA_CONF_DEFAULT, gerr->message);
> >> +        exit(EXIT_FAILURE);
> >> +    }
> >> +    g_clear_error(&gerr);
> >
> > What about other file errors, like permission issues? Maybe just have
> > config_load() return bool, and just spit out stringified gerr prior to
> > return false?
> 
> all errors have a g_critical(), except for the case of file missing.

Argh, missed the '!'. Looks good then:

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> 
> 
> -- 
> Marc-André Lureau
>
Markus Armbruster Aug. 26, 2015, 8:01 a.m. UTC | #4 
marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Learn to configure the agent with a system configuration.
>
> This may simplify command-line handling, especially when the blacklist
> is long.
>
> Among the other benefits, this may standardize the configuration of a
> init service (instead of distro-specific init keys/files)
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

This uses GLib's Key-value file parser.  Note that we have our own .ini
parser qemu_config_parse().  It predates our use of GLib.  Having two
different parsers risks inconsistency.  Since qga is already using
GLib's, using it some more there is better than adding a use of our own
parser, so no objection to your patch on that ground.

Replacing qemu_config_parse()'s parsing guts by GLib probably won't save
code, but it could be nice for consistency.  Well outside this patch's
scope, of course.
diff mbox

Patch

diff --git a/qga/main.c b/qga/main.c
index 58f2fc7..154975c 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -56,6 +56,7 @@ 
 #define QGA_FSFREEZE_HOOK_DEFAULT CONFIG_QEMU_CONFDIR "/fsfreeze-hook"
 #endif
 #define QGA_SENTINEL_BYTE 0xFF
+#define QGA_CONF_DEFAULT CONFIG_QEMU_CONFDIR G_DIR_SEPARATOR_S "qemu-ga.conf"
 
 static struct {
     const char *state_dir;
@@ -936,14 +937,80 @@  typedef struct GAConfig {
 #ifdef _WIN32
     const char *service;
 #endif
+    gchar *bliststr;
     GList *blacklist;
     int daemonize;
     GLogLevelFlags log_level;
 } GAConfig;
 
-static GAConfig *config_parse(int argc, char **argv)
+static void config_load(GAConfig *config)
+{
+    GError *gerr = NULL;
+    GKeyFile *keyfile;
+
+    /* read system config */
+    keyfile = g_key_file_new();
+    if (!g_key_file_load_from_file(keyfile, QGA_CONF_DEFAULT, 0, &gerr)) {
+        goto end;
+    }
+    if (g_key_file_has_key(keyfile, "general", "daemon", NULL)) {
+        config->daemonize =
+            g_key_file_get_boolean(keyfile, "general", "daemon", &gerr);
+    }
+    if (g_key_file_has_key(keyfile, "general", "method", NULL)) {
+        config->method =
+            g_key_file_get_string(keyfile, "general", "method", &gerr);
+    }
+    if (g_key_file_has_key(keyfile, "general", "path", NULL)) {
+        config->channel_path =
+            g_key_file_get_string(keyfile, "general", "path", &gerr);
+    }
+    if (g_key_file_has_key(keyfile, "general", "logfile", NULL)) {
+        config->log_filepath =
+            g_key_file_get_string(keyfile, "general", "logfile", &gerr);
+    }
+    if (g_key_file_has_key(keyfile, "general", "pidfile", NULL)) {
+        config->pid_filepath =
+            g_key_file_get_string(keyfile, "general", "pidfile", &gerr);
+    }
+#ifdef CONFIG_FSFREEZE
+    if (g_key_file_has_key(keyfile, "general", "fsfreeze-hook", NULL)) {
+        config->fsfreeze_hook =
+            g_key_file_get_string(keyfile,
+                                  "general", "fsfreeze-hook", &gerr);
+    }
+#endif
+    if (g_key_file_has_key(keyfile, "general", "statedir", NULL)) {
+        config->state_dir =
+            g_key_file_get_string(keyfile, "general", "statedir", &gerr);
+    }
+    if (g_key_file_has_key(keyfile, "general", "verbose", NULL) &&
+        g_key_file_get_boolean(keyfile, "general", "verbose", &gerr)) {
+        /* enable all log levels */
+        config->log_level = G_LOG_LEVEL_MASK;
+    }
+    if (g_key_file_has_key(keyfile, "general", "blacklist", NULL)) {
+        config->bliststr =
+            g_key_file_get_string(keyfile, "general", "blacklist", &gerr);
+        config->blacklist = g_list_concat(config->blacklist,
+                                          split_list(config->bliststr, ','));
+    }
+
+end:
+    if (keyfile) {
+        g_key_file_free(keyfile);
+    }
+    if (gerr &&
+        !(gerr->domain == G_FILE_ERROR && gerr->code == G_FILE_ERROR_NOENT)) {
+        g_critical("error loading configuration from path: %s, %s",
+                   QGA_CONF_DEFAULT, gerr->message);
+        exit(EXIT_FAILURE);
+    }
+    g_clear_error(&gerr);
+}
+
+static void config_parse(GAConfig *config, int argc, char **argv)
 {
-    GAConfig *config = g_new0(GAConfig, 1);
     const char *sopt = "hVvdm:p:l:f:F::b:s:t:D";
     int opt_ind = 0, ch;
     const struct option lopt[] = {
@@ -1047,8 +1114,6 @@  static GAConfig *config_parse(int argc, char **argv)
             exit(EXIT_FAILURE);
         }
     }
-
-    return config;
 }
 
 static void config_free(GAConfig *config)
@@ -1058,6 +1123,7 @@  static void config_free(GAConfig *config)
     g_free(config->pid_filepath);
     g_free(config->state_dir);
     g_free(config->channel_path);
+    g_free(config->bliststr);
 #ifdef CONFIG_FSFREEZE
     g_free(config->fsfreeze_hook);
 #endif
@@ -1200,13 +1266,13 @@  int main(int argc, char **argv)
 {
     int ret = EXIT_SUCCESS;
     GAState *s = g_new0(GAState, 1);
-    GAConfig *config;
+    GAConfig *config = g_new0(GAConfig, 1);
 
     module_call_init(MODULE_INIT_QAPI);
 
     init_dfl_pathnames();
-
-    config = config_parse(argc, argv);
+    config_load(config);
+    config_parse(config, argc, argv);
 
     if (config->pid_filepath == NULL) {
         config->pid_filepath = g_strdup(dfl_pathnames.pidfile);