Hi
----- Original Message -----
> On 08/26/2015 01:05 PM, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lur...@redhat.com>
> >
> > A following patch will return allocated string.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> > Reviewed-by: Michael Roth <mdr...@linux.vnet.ibm.com>
> > ---
> > 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 = "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
> > @@ -981,31 +981,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;
> > @@ -1028,20 +1025,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)) {
>
> here the original behaviour is silently changed
>
> > exit(EXIT_FAILURE);
> > }
> > exit(EXIT_SUCCESS);
> > @@ -1072,6 +1059,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
> > @@ -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
allocated strings.
> These bits will not make next patch shorter. It would be the same.
> The description could be clear with this approach
