From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52077) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUfB9-00067m-I3 for qemu-devel@nongnu.org; Wed, 26 Aug 2015 14:12:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZUfB6-0005Ed-9M for qemu-devel@nongnu.org; Wed, 26 Aug 2015 14:12:07 -0400 Received: from mx2.parallels.com ([199.115.105.18]:48276) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUfB6-0005Dj-1k for qemu-devel@nongnu.org; Wed, 26 Aug 2015 14:12:04 -0400 References: <1440583525-21632-1-git-send-email-marcandre.lureau@redhat.com> <1440583525-21632-7-git-send-email-marcandre.lureau@redhat.com> From: "Denis V. Lunev" Message-ID: <55DE016C.3070000@parallels.com> Date: Wed, 26 Aug 2015 21:11:56 +0300 MIME-Version: 1.0 In-Reply-To: <1440583525-21632-7-git-send-email-marcandre.lureau@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v3 06/12] qga: move option parsing to separate function List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: marcandre.lureau@redhat.com, qemu-devel@nongnu.org Cc: mdroth@linux.vnet.ibm.com On 08/26/2015 01:05 PM, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau > > Move option parsing out of giant main(). > > Signed-off-by: Marc-André Lureau > Reviewed-by: Michael Roth > --- > qga/main.c | 165 +++++++++++++++++++++++++++++++++++-------------------------- > 1 file changed, 96 insertions(+), 69 deletions(-) > > diff --git a/qga/main.c b/qga/main.c > index 83b7804..8c4a075 100644 > --- a/qga/main.c > +++ b/qga/main.c > @@ -941,19 +941,28 @@ static GList *split_list(gchar *str, const gchar separator) > return list; > } > > -int main(int argc, char **argv) > -{ > - const char *sopt = "hVvdm:p:l:f:F::b:s:t:"; > - char *method = NULL, *channel_path = NULL; > - char *log_filepath = NULL; > - char *pid_filepath = NULL; > +typedef struct GAConfig { > + char *channel_path; > + char *method; > + char *log_filepath; > + char *pid_filepath; > #ifdef CONFIG_FSFREEZE > - char *fsfreeze_hook = NULL; > + char *fsfreeze_hook; > #endif > - char *state_dir = NULL; > + char *state_dir; > #ifdef _WIN32 > - const char *service = NULL; > + const char *service; > #endif > + GList *blacklist; > + int daemonize; > + GLogLevelFlags log_level; > +} GAConfig; > + > +static GAConfig *config_parse(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[] = { > { "help", 0, NULL, 'h' }, > { "version", 0, NULL, 'V' }, > @@ -973,74 +982,71 @@ int main(int argc, char **argv) > { "statedir", 1, NULL, 't' }, > { NULL, 0, NULL, 0 } > }; > - int opt_ind = 0, ch, daemonize = 0; > - GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL; > - GList *blacklist = NULL; > - GAState *s; > > - module_call_init(MODULE_INIT_QAPI); > + config->log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL; > > - init_dfl_pathnames(); > while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) { > switch (ch) { > case 'm': > - method = g_strdup(optarg); > + config->method = g_strdup(optarg); > break; > case 'p': > - channel_path = g_strdup(optarg); > + config->channel_path = g_strdup(optarg); > break; > case 'l': > - log_filepath = g_strdup(optarg); > + config->log_filepath = g_strdup(optarg); > break; > case 'f': > - pid_filepath = g_strdup(optarg); > + config->pid_filepath = g_strdup(optarg); > break; > #ifdef CONFIG_FSFREEZE > case 'F': > - fsfreeze_hook = g_strdup(optarg ?: QGA_FSFREEZE_HOOK_DEFAULT); > + config->fsfreeze_hook = g_strdup(optarg ?: QGA_FSFREEZE_HOOK_DEFAULT); > break; > #endif > case 't': > - state_dir = g_strdup(optarg); > + config->state_dir = g_strdup(optarg); > break; > case 'v': > /* enable all log levels */ > - log_level = G_LOG_LEVEL_MASK; > + config->log_level = G_LOG_LEVEL_MASK; > break; > case 'V': > printf("QEMU Guest Agent %s\n", QEMU_VERSION); > exit(EXIT_SUCCESS); > case 'd': > - daemonize = 1; > + config->daemonize = 1; > break; > case 'b': { > if (is_help_option(optarg)) { > qmp_for_each_command(ga_print_cmd, NULL); > exit(EXIT_SUCCESS); > } > - blacklist = g_list_concat(blacklist, split_list(optarg, ',')); > + config->blacklist = g_list_concat(config->blacklist, > + split_list(optarg, ',')); > break; > } > #ifdef _WIN32 > case 's': > - service = optarg; > - if (strcmp(service, "install") == 0) { > + config->service = optarg; > + if (strcmp(config->service, "install") == 0) { > if (ga_install_vss_provider()) { > exit(EXIT_FAILURE); > } > - if (ga_install_service(channel_path, log_filepath, state_dir)) { > + if (ga_install_service(config->channel_path, > + config->log_filepath, config->state_dir)) { > exit(EXIT_FAILURE); > } > exit(EXIT_SUCCESS); > - } else if (strcmp(service, "uninstall") == 0) { > + } else if (strcmp(config->service, "uninstall") == 0) { > ga_uninstall_vss_provider(); > exit(ga_uninstall_service()); > - } else if (strcmp(service, "vss-install") == 0) { > + } else if (strcmp(config->service, "vss-install") == 0) { > if (ga_install_vss_provider()) { > exit(EXIT_FAILURE); > } > exit(EXIT_SUCCESS); > - } else if (strcmp(service, "vss-uninstall") == 0) { > + } else if (strcmp(config->service, "vss-uninstall") == 0) { > ga_uninstall_vss_provider(); > exit(EXIT_SUCCESS); > } else { > @@ -1059,12 +1065,39 @@ int main(int argc, char **argv) > } > } > > - if (pid_filepath == NULL) { > - pid_filepath = g_strdup(dfl_pathnames.pidfile); > + return config; > +} > + > +static void config_free(GAConfig *config) > +{ > + g_free(config->method); > + g_free(config->log_filepath); > + g_free(config->pid_filepath); > + g_free(config->state_dir); > + g_free(config->channel_path); > +#ifdef CONFIG_FSFREEZE > + g_free(config->fsfreeze_hook); > +#endif > + g_free(config); > +} > + > +int main(int argc, char **argv) > +{ > + GAState *s; > + GAConfig *config; > + > + module_call_init(MODULE_INIT_QAPI); > + > + init_dfl_pathnames(); > + > + config = config_parse(argc, argv); > + > + if (config->pid_filepath == NULL) { > + config->pid_filepath = g_strdup(dfl_pathnames.pidfile); > } > > - if (state_dir == NULL) { > - state_dir = g_strdup(dfl_pathnames.state_dir); > + if (config->state_dir == NULL) { > + config->state_dir = g_strdup(dfl_pathnames.state_dir); > } > > #ifdef _WIN32 > @@ -1074,25 +1107,25 @@ int main(int argc, char **argv) > * error later on, we won't try to clean up the directory, it is considered > * persistent. > */ > - if (g_mkdir_with_parents(state_dir, S_IRWXU) == -1) { > + if (g_mkdir_with_parents(config->state_dir, S_IRWXU) == -1) { > g_critical("unable to create (an ancestor of) the state directory" > - " '%s': %s", state_dir, strerror(errno)); > + " '%s': %s", config->state_dir, strerror(errno)); > return EXIT_FAILURE; > } > #endif > > s = g_malloc0(sizeof(GAState)); > - s->log_level = log_level; > + s->log_level = config->log_level; > s->log_file = stderr; > #ifdef CONFIG_FSFREEZE > - s->fsfreeze_hook = fsfreeze_hook; > + s->fsfreeze_hook = config->fsfreeze_hook; > #endif > g_log_set_default_handler(ga_log, s); > g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR); > ga_enable_logging(s); > s->state_filepath_isfrozen = g_strdup_printf("%s/qga.state.isfrozen", > - state_dir); > - s->pstate_filepath = g_strdup_printf("%s/qga.state", state_dir); > + config->state_dir); > + s->pstate_filepath = g_strdup_printf("%s/qga.state", config->state_dir); > s->frozen = false; > > #ifndef _WIN32 > @@ -1125,23 +1158,23 @@ int main(int argc, char **argv) > #endif > > if (ga_is_frozen(s)) { > - if (daemonize) { > + if (config->daemonize) { > /* delay opening/locking of pidfile till filesystems are unfrozen */ > - s->deferred_options.pid_filepath = pid_filepath; > + s->deferred_options.pid_filepath = config->pid_filepath; > become_daemon(NULL); > } > - if (log_filepath) { > + if (config->log_filepath) { > /* delay opening the log file till filesystems are unfrozen */ > - s->deferred_options.log_filepath = log_filepath; > + s->deferred_options.log_filepath = config->log_filepath; > } > ga_disable_logging(s); > qmp_for_each_command(ga_disable_non_whitelisted, NULL); > } else { > - if (daemonize) { > - become_daemon(pid_filepath); > + if (config->daemonize) { > + become_daemon(config->pid_filepath); > } > - if (log_filepath) { > - FILE *log_file = ga_open_logfile(log_filepath); > + if (config->log_filepath) { > + FILE *log_file = ga_open_logfile(config->log_filepath); > if (!log_file) { > g_critical("unable to open specified log file: %s", > strerror(errno)); > @@ -1159,14 +1192,15 @@ int main(int argc, char **argv) > goto out_bad; > } > > - blacklist = ga_command_blacklist_init(blacklist); > - if (blacklist) { > - s->blacklist = blacklist; > + config->blacklist = ga_command_blacklist_init(config->blacklist); > + if (config->blacklist) { > + GList *l = config->blacklist; > + s->blacklist = config->blacklist; > do { > - g_debug("disabling command: %s", (char *)blacklist->data); > - qmp_disable_command(blacklist->data); > - blacklist = g_list_next(blacklist); > - } while (blacklist); > + g_debug("disabling command: %s", (char *)l->data); > + qmp_disable_command(l->data); > + l = g_list_next(l); > + } while (l); > } > s->command_state = ga_command_state_new(); > ga_command_state_init(s, s->command_state); > @@ -1181,14 +1215,14 @@ int main(int argc, char **argv) > #endif > > s->main_loop = g_main_loop_new(NULL, false); > - if (!channel_init(ga_state, method, channel_path)) { > + if (!channel_init(ga_state, config->method, config->channel_path)) { > g_critical("failed to initialize guest agent channel"); > goto out_bad; > } > #ifndef _WIN32 > g_main_loop_run(ga_state->main_loop); > #else > - if (daemonize) { > + if (config->daemonize) { > SERVICE_TABLE_ENTRY service_table[] = { > { (char *)QGA_SERVICE_NAME, service_main }, { NULL, NULL } }; > StartServiceCtrlDispatcher(service_table); > @@ -1200,24 +1234,17 @@ int main(int argc, char **argv) > ga_command_state_cleanup_all(ga_state->command_state); > ga_channel_free(ga_state->channel); > > - if (daemonize) { > - unlink(pid_filepath); > + if (config->daemonize) { > + unlink(config->pid_filepath); > } > return 0; > > out_bad: > - if (daemonize) { > - unlink(pid_filepath); > + if (config->daemonize) { > + unlink(config->pid_filepath); > } > > - g_free(method); > - g_free(log_filepath); > - g_free(pid_filepath); > - g_free(state_dir); > - g_free(channel_path); > -#ifdef CONFIG_FSFREEZE > - g_free(fsfreeze_hook); > -#endif > + config_free(config); > > return EXIT_FAILURE; > } Reviewed-by: Denis V. Lunev