Re: [Qemu-devel] [PATCH v2 10/12] qga: add an optionnal qemu-ga.conf system configuration

[Michael Roth]

Quoting address@hidden (2015-08-25 17:10:22)
> From: Marc-André Lureau <address@hidden>
> 
> 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 <address@hidden>
> ---
>  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
>