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

[Michael Roth <mdroth@linux.vnet.ibm.com>]

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
>