Message ID | 1440540624-7998-11-git-send-email-marcandre.lureau@redhat.com |
---|---|
State | New |
Headers | show |
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 >
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.
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 >
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 --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);