From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55639) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUtsA-0006jg-F7 for qemu-devel@nongnu.org; Thu, 27 Aug 2015 05:53:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZUts7-0001mN-Kw for qemu-devel@nongnu.org; Thu, 27 Aug 2015 05:53:30 -0400 Received: from mx2.parallels.com ([199.115.105.18]:33838) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUts7-0001jB-Ci for qemu-devel@nongnu.org; Thu, 27 Aug 2015 05:53:27 -0400 References: <1440632099-5169-1-git-send-email-marcandre.lureau@redhat.com> <1440632099-5169-7-git-send-email-marcandre.lureau@redhat.com> From: "Denis V. Lunev" Message-ID: <55DEDE05.9060707@openvz.org> Date: Thu, 27 Aug 2015 12:53:09 +0300 MIME-Version: 1.0 In-Reply-To: <1440632099-5169-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 v4 06/13] qga: copy argument strings 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/27/2015 02:34 AM, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau > > Following patch will return allocated strings, so we must correctly > initialize alloc & free them. The nice side effect is that we no longer > have to check for "fixed_state_dir" to call ga_install_service() with a > NULL state dir. The default values are set after parsing the command > line options. > > Signed-off-by: Marc-André Lureau > Reviewed-by: Michael Roth > --- > qga/main.c | 57 +++++++++++++++++++++++++++++++-------------------------- > 1 file changed, 31 insertions(+), 26 deletions(-) > > diff --git a/qga/main.c b/qga/main.c > index 7fa6dcc..766cb93 100644 > --- a/qga/main.c > +++ b/qga/main.c > @@ -939,13 +939,13 @@ static GList *split_list(const gchar *str, const gchar *delim) > int main(int argc, char **argv) > { > const char *sopt = "hVvdm:p:l:f:F::b:s:t:"; > - const char *method = NULL, *channel_path = NULL; > - const char *log_filepath = NULL; > - const char *pid_filepath; > + char *method = NULL, *channel_path = NULL; > + char *log_filepath = NULL; > + char *pid_filepath = NULL; > #ifdef CONFIG_FSFREEZE > - const char *fsfreeze_hook = NULL; > + char *fsfreeze_hook = NULL; > #endif > - const char *state_dir; > + char *state_dir = NULL; > #ifdef _WIN32 > const char *service = NULL; > #endif > @@ -976,31 +976,28 @@ int main(int argc, char **argv) > module_call_init(MODULE_INIT_QAPI); > > init_dfl_pathnames(); > - pid_filepath = dfl_pathnames.pidfile; > - state_dir = dfl_pathnames.state_dir; > - > while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) { > switch (ch) { > case 'm': > - method = optarg; > + method = g_strdup(optarg); > break; > case 'p': > - channel_path = optarg; > + channel_path = g_strdup(optarg); > break; > case 'l': > - log_filepath = optarg; > + log_filepath = g_strdup(optarg); > break; > case 'f': > - pid_filepath = optarg; > + pid_filepath = g_strdup(optarg); > break; > #ifdef CONFIG_FSFREEZE > case 'F': > - fsfreeze_hook = optarg ? optarg : QGA_FSFREEZE_HOOK_DEFAULT; > + fsfreeze_hook = g_strdup(optarg ?: QGA_FSFREEZE_HOOK_DEFAULT); > break; > #endif > case 't': > - state_dir = optarg; > - break; > + state_dir = g_strdup(optarg); > + break; > case 'v': > /* enable all log levels */ > log_level = G_LOG_LEVEL_MASK; > @@ -1023,20 +1020,10 @@ int main(int argc, char **argv) > case 's': > service = optarg; > if (strcmp(service, "install") == 0) { > - const char *fixed_state_dir; > - > - /* If the user passed the "-t" option, we save that state dir > - * in the service. Otherwise we let the service fetch the state > - * dir from the environment when it starts. > - */ > - fixed_state_dir = (state_dir == dfl_pathnames.state_dir) ? > - NULL : > - state_dir; > if (ga_install_vss_provider()) { > exit(EXIT_FAILURE); > } > - if (ga_install_service(channel_path, log_filepath, > - fixed_state_dir)) { > + if (ga_install_service(channel_path, log_filepath, state_dir)) { > exit(EXIT_FAILURE); > } > exit(EXIT_SUCCESS); > @@ -1067,6 +1054,14 @@ int main(int argc, char **argv) > } > } > > + if (pid_filepath == NULL) { > + pid_filepath = g_strdup(dfl_pathnames.pidfile); > + } > + > + if (state_dir == NULL) { > + state_dir = g_strdup(dfl_pathnames.state_dir); > + } > + > #ifdef _WIN32 > /* On win32 the state directory is application specific (be it the default > * or a user override). We got past the command line parsing; let's create > @@ -1210,5 +1205,15 @@ out_bad: > if (daemonize) { > unlink(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 > + > return EXIT_FAILURE; > } Reviewed-by: Denis V. Lunev