Re: [Qemu-devel] [PATCH v3 05/12] qga: copy argument strings

[Marc-André Lureau]

Hi

----- Original Message -----
> On 08/26/2015 01:05 PM, address@hidden wrote:
> > From: Marc-André Lureau <address@hidden>
> >
> > A following patch will return allocated string.
> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > Reviewed-by: Michael Roth <address@hidden>
> > ---
> >   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