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

["Denis V. Lunev" <den-lists@parallels.com>]

On 08/26/2015 01:05 PM, marcandre.lureau@redhat.com wrote:
> 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>
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>   qga/main.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 73 insertions(+), 7 deletions(-)
>
> diff --git a/qga/main.c b/qga/main.c
> index 58f2fc7..9193043 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; /* blacklist may point to this string */
>       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);
sequential calling of config_load/config_parse
means memory leak in config_parse if the
option is present in the file and overwritten
during parsing.