From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57822) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUfQT-0008Kl-Rh for qemu-devel@nongnu.org; Wed, 26 Aug 2015 14:27:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZUfQO-0000rQ-RW for qemu-devel@nongnu.org; Wed, 26 Aug 2015 14:27:57 -0400 Received: from mx2.parallels.com ([199.115.105.18]:49201) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUfQO-0000rM-J1 for qemu-devel@nongnu.org; Wed, 26 Aug 2015 14:27:52 -0400 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> <1569455872.1683587.1440613054232.JavaMail.zimbra@redhat.com> From: "Denis V. Lunev" Message-ID: <55DE051D.60805@parallels.com> Date: Wed, 26 Aug 2015 21:27:41 +0300 MIME-Version: 1.0 In-Reply-To: <1569455872.1683587.1440613054232.JavaMail.zimbra@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit 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: =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= Cc: marcandre lureau , qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.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 >>> >>> A following patch will return allocated string. >>> >>> 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 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