From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54262) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUfGW-0003ej-TY for qemu-devel@nongnu.org; Wed, 26 Aug 2015 14:17:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZUfGT-0002S1-My for qemu-devel@nongnu.org; Wed, 26 Aug 2015 14:17:40 -0400 Received: from mx4-phx2.redhat.com ([209.132.183.25]:41849) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUfGT-0002Rf-Fp for qemu-devel@nongnu.org; Wed, 26 Aug 2015 14:17:37 -0400 Date: Wed, 26 Aug 2015 14:17:34 -0400 (EDT) From: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Message-ID: <1569455872.1683587.1440613054232.JavaMail.zimbra@redhat.com> In-Reply-To: <55DE00EE.9080600@parallels.com> References: <1440583525-21632-1-git-send-email-marcandre.lureau@redhat.com> <1440583525-21632-6-git-send-email-marcandre.lureau@redhat.com> <55DE00EE.9080600@parallels.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 05/12] qga: copy argument strings List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" Cc: marcandre lureau , qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com Hi ----- Original Message ----- > On 08/26/2015 01:05 PM, marcandre.lureau@redhat.com wrote: > > From: Marc-Andr=C3=A9 Lureau > > > > A following patch will return allocated string. > > > > Signed-off-by: Marc-Andr=C3=A9 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 ede5306..83b7804 100644 > > --- a/qga/main.c > > +++ b/qga/main.c > > @@ -944,13 +944,13 @@ static GList *split_list(gchar *str, const gchar > > separator) > > int main(int argc, char **argv) > > { > > const char *sopt =3D "hVvdm:p:l:f:F::b:s:t:"; > > - const char *method =3D NULL, *channel_path =3D NULL; > > - const char *log_filepath =3D NULL; > > - const char *pid_filepath; > > + char *method =3D NULL, *channel_path =3D NULL; > > + char *log_filepath =3D NULL; > > + char *pid_filepath =3D NULL; > > #ifdef CONFIG_FSFREEZE > > - const char *fsfreeze_hook =3D NULL; > > + char *fsfreeze_hook =3D NULL; > > #endif > > - const char *state_dir; > > + char *state_dir =3D NULL; > > #ifdef _WIN32 > > const char *service =3D NULL; > > #endif > > @@ -981,31 +981,28 @@ int main(int argc, char **argv) > > module_call_init(MODULE_INIT_QAPI); > > =20 > > init_dfl_pathnames(); > > - pid_filepath =3D dfl_pathnames.pidfile; > > - state_dir =3D dfl_pathnames.state_dir; > > - > > while ((ch =3D getopt_long(argc, argv, sopt, lopt, &opt_ind)) != =3D -1) { > > switch (ch) { > > case 'm': > > - method =3D optarg; > > + method =3D g_strdup(optarg); > > break; > > case 'p': > > - channel_path =3D optarg; > > + channel_path =3D g_strdup(optarg); > > break; > > case 'l': > > - log_filepath =3D optarg; > > + log_filepath =3D g_strdup(optarg); > > break; > > case 'f': > > - pid_filepath =3D optarg; > > + pid_filepath =3D g_strdup(optarg); > > break; > > #ifdef CONFIG_FSFREEZE > > case 'F': > > - fsfreeze_hook =3D optarg ? optarg : QGA_FSFREEZE_HOOK_DEFA= ULT; > > + fsfreeze_hook =3D g_strdup(optarg ?: QGA_FSFREEZE_HOOK_DEF= AULT); > > break; > > #endif > > case 't': > > - state_dir =3D optarg; > > - break; > > + state_dir =3D g_strdup(optarg); > > + break; > > case 'v': > > /* enable all log levels */ > > log_level =3D G_LOG_LEVEL_MASK; > > @@ -1028,20 +1025,10 @@ int main(int argc, char **argv) > > case 's': > > service =3D optarg; > > if (strcmp(service, "install") =3D=3D 0) { > > - const char *fixed_state_dir; > > - > > - /* If the user passed the "-t" option, we save that st= ate > > dir > > - * in the service. Otherwise we let the service fetch = the > > state > > - * dir from the environment when it starts. > > - */ > > - fixed_state_dir =3D (state_dir =3D=3D dfl_pathnames.st= ate_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)) { >=20 > here the original behaviour is silently changed >=20 > > exit(EXIT_FAILURE); > > } > > exit(EXIT_SUCCESS); > > @@ -1072,6 +1059,14 @@ int main(int argc, char **argv) > > } > > } > > =20 > > + if (pid_filepath =3D=3D NULL) { > > + pid_filepath =3D g_strdup(dfl_pathnames.pidfile); > > + } > > + > > + if (state_dir =3D=3D NULL) { > > + state_dir =3D g_strdup(dfl_pathnames.state_dir); > > + } > > + > > #ifdef _WIN32 > > /* On win32 the state directory is application specific (be it th= e > > default > > * or a user override). We got past the command line parsing; let= 's > > create > > @@ -1214,5 +1209,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; > > } > I think that patch in this form is overkill. IMHO it would be shorter > to change initialization sequence clearing pid_filepath and > state_dir assignment and not play with g_free/g_strdup Sorry, I don't understand. do you mean it shouldn't use strdup/free at all?= that's not possible without leaking in the further patches that return all= ocated strings. > These bits will not make next patch shorter. It would be the same. > The description could be clear with this approach