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

["Denis V. Lunev" <den-lists@parallels.com>]

On 08/26/2015 09:17 PM, Marc-André Lureau wrote:
> Hi
>
> ----- Original Message -----
>> On 08/26/2015 01:05 PM, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> A following patch will return allocated string.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> Reviewed-by: Michael Roth <mdroth@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
>>
I was wrong here, pls ignore..


>>>                        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
>
lets consider this patch. You have done 2 things:
- changed initialisation order and dropped nasty temporary variables
- introduced alloc/free code

But in the next patch each line with alloc/free code
will be changed due to variable rename and
moving to the separate function (free), which
IMHO means that this preparatory step is unnecessary,
you will make almost same changes in the next
patch

Thus the sum of changes in this patch/next patch
would be less with uncovering initialization
change details, which are missed in the patch
description