On 08/26/2015 09:17 PM, Marc-André Lureau wrote:
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
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
