[lttng-dev] [PATCH] lttng-tools lttng commands add_context.c: Plug memory leaks

[lttng-dev] [PATCH] lttng-tools lttng commands add_context.c: Plug memory leaks

[Thibault, Daniel]

Re: [lttng-dev] [PATCH] lttng-tools lttng commands add_context.c: Plug memory leaks
Message-ID: <20120126193128.GA14376 at Krystal>

> Please always use
>
> /*
>  * My comment.
>  * For a multi-line comment.
>  */
>
> style of comment or
>
> /* My comment. For single-line comment. */
>
> Mathieu

   Noted.  Do I need to resubmit the patch?

Daniel U. Thibault
R & D pour la d?fense Canada - Valcartier (RDDC Valcartier) / Defence R&D Canada - Valcartier (DRDC Valcartier)
Syst?me de syst?mes (SdS) / System of Systems (SoS)
Solutions informatiques et exp?rimentations (SIE) / Computing Solutions and Experimentations (CSE)
2459 Boul. Pie XI Nord
Qu?bec, QC  G3J 1X5
CANADA
Vox : (418) 844-4000 x4245
Fax : (418) 844-4538
NAC: 918V QSDJ
Gouvernement du Canada / Government of Canada
<http://www.valcartier.drdc-rddc.gc.ca/>

[lttng-dev] [PATCH] lttng-tools lttng commands calibrate.c : Return values of the cmd_calibrate() chain of functions

[Thibault, Daniel]

Improve lttng/commands/calibrate.c usage() printout.
Document and enforce the expected return values for the cmd_calibrate() chain of functions.
This includes a couple of documentation lines in lib/lttng-ctl/lttng-ctl.c.
------------------------------
From 4c9089ec185d72c9e457dc6656323ae0e2939ae3 Thu, 26 Jan 2012 16:05:04 -0500
From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
Date: Thu, 26 Jan 2012 16:04:41 -0500
Subject: [PATCH] lttng-tools lttng commands calibrate.c : Return values of the cmd_calibrate() chain of functions

diff --git a/src/bin/lttng/commands/calibrate.c b/src/bin/lttng/commands/calibrate.c
index 81eadbf..1af1807 100644
--- a/src/bin/lttng/commands/calibrate.c
+++ b/src/bin/lttng/commands/calibrate.c
@@ -89,14 +89,14 @@
 	fprintf(ofp, "\n");
 	fprintf(ofp, "  -h, --help               Show this help\n");
 	fprintf(ofp, "      --list-options       Simple listing of options\n");
-	fprintf(ofp, "  -k, --kernel             Apply for the kernel tracer\n");
+	fprintf(ofp, "  -k, --kernel             Apply to the kernel tracer\n");
 #if 0
-	fprintf(ofp, "  -u, --userspace [CMD]    Apply for the user-space tracer\n");
-	fprintf(ofp, "                           If no CMD, the domain used is UST global\n");
-	fprintf(ofp, "                           or else the domain is UST EXEC_NAME\n");
+	fprintf(ofp, "  -u, --userspace [CMD]    Apply to the user-space tracer (domain: UST\n");
+	fprintf(ofp, "                           EXEC_NAME). If no CMD, the domain is UST globa.\n");
+	fprintf(ofp, "                           (-k preempts -u)\n");
 	fprintf(ofp, "  -p, --pid PID            If -u, apply to specific PID (domain: UST PID)\n");
 #else
-	fprintf(ofp, "  -u, --userspace          Apply for the user-space tracer\n");
+	fprintf(ofp, "  -u, --userspace          Apply to the user-space tracer\n");
 #endif
 	fprintf(ofp, "\n");
 	fprintf(ofp, "Calibrate options:\n");
@@ -120,6 +120,7 @@
  *  calibrate_lttng
  *
  *  Calibrate LTTng.
+ *  Returns one of the CMD_* result constants.
  */
 static int calibrate_lttng(void)
 {
@@ -140,7 +141,7 @@
 
 	handle = lttng_create_handle(NULL, &dom);
 	if (handle == NULL) {
-		ret = -1;
+		ret = CMD_ERROR;
 		goto error;
 	}
 
@@ -154,9 +155,17 @@
 	case LTTNG_EVENT_FUNCTION:
 		DBG("Calibrating kernel functions");
 		calibrate.type = LTTNG_CALIBRATE_FUNCTION;
+		/*
+		 * lttng_calibrate() returns the size of
+		 * the returned session payload data
+		 * or a negative error code.
+		 */
 		ret = lttng_calibrate(handle, &calibrate);
 		if (ret < 0) {
+			ret = CMD_ERROR;
 			goto error;
+		} else {
+			ret = CMD_SUCCESS;
 		}
 		MSG("%s calibration done", opt_kernel ? "Kernel" : "UST");
 		break;
@@ -181,6 +190,7 @@
  *  cmd_calibrate
  *
  *  Calibrate LTTng tracer.
+ *  Returns one of the CMD_* result constants.
  */
 int cmd_calibrate(int argc, const char **argv)
 {
@@ -196,7 +206,7 @@
 	while ((opt = poptGetNextOpt(pc)) != -1) {
 		switch (opt) {
 		case OPT_HELP:
-			usage(stderr);
+			usage(stdout);
 			ret = CMD_SUCCESS;
 			goto end;
 		case OPT_TRACEPOINT:
diff --git a/src/lib/lttng-ctl/lttng-ctl.c b/src/lib/lttng-ctl/lttng-ctl.c
index b7fc781..e7ebab5 100644
--- a/src/lib/lttng-ctl/lttng-ctl.c
+++ b/src/lib/lttng-ctl/lttng-ctl.c
@@ -302,7 +302,7 @@
 /*
  * Ask the session daemon a specific command and put the data into buf.
  *
- * Return size of data (only payload, not header).
+ * Return size of data (only payload, not header) or a negative error code.
  */
 static int ask_sessiond(struct lttcomm_session_msg *lsm, void **buf)
 {
@@ -808,6 +808,7 @@
 
 /*
  *  lttng_calibrate
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_calibrate(struct lttng_handle *handle,
 		struct lttng_calibrate *calibrate)
------------------------------

Daniel U. Thibault
R & D pour la d?fense Canada - Valcartier (RDDC Valcartier) / Defence R&D Canada - Valcartier (DRDC Valcartier)
Syst?me de syst?mes (SdS) / System of Systems (SoS)
Solutions informatiques et exp?rimentations (SIE) / Computing Solutions and Experimentations (CSE)
2459 Boul. Pie XI Nord
Qu?bec, QC? G3J 1X5
CANADA
Vox : (418) 844-4000 x4245
Fax : (418) 844-4538
NAC: 918V QSDJ
Gouvernement du Canada?/ Government of Canada
<http://www.valcartier.drdc-rddc.gc.ca/>

[lttng-dev] [PATCH] lttng-tools lttng commands add_context.c : Return values of the cmd_add_context() chain of functions

[Thibault, Daniel]

Document and ensure the return values of the cmd_add_context() chain of functions.
Make add_context() stop looping over lttng_add_context() upon failure (otherwise it reports success or failure depending only on the last call's result).
Also patches lib/lttng-ctl/lttng-ctl.c
------------------------------
From 9de81398942fe0adeb5288483ecb70e2573e69f0 Thu, 26 Jan 2012 16:23:05 -0500
From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
Date: Thu, 26 Jan 2012 16:22:54 -0500
Subject: [PATCH] lttng-tools lttng commands add_context.c : Return values of the cmd_add_context() chain of functions

diff --git a/src/bin/lttng/commands/add_context.c b/src/bin/lttng/commands/add_context.c
index d0f5b6f..1c63d90 100644
--- a/src/bin/lttng/commands/add_context.c
+++ b/src/bin/lttng/commands/add_context.c
@@ -358,6 +358,7 @@
 
 /*
  * Add context to channel or event.
+ * Returns one of the CMD_* result constants.
  */
 static int add_context(char *session_name)
 {
@@ -379,7 +380,7 @@
 
 	handle = lttng_create_handle(session_name, &dom);
 	if (handle == NULL) {
-		ret = -1;
+		ret = CMD_ERROR;
 		goto error;
 	}
 
@@ -400,11 +401,17 @@
 		}
 		DBG("Adding context...");
 
+		/*
+		 * lttng_add_context() returns the size
+		 * of the returned payload data
+		 * or a negative error code.
+		 */
 		ret = lttng_add_context(handle, &context, opt_event_name,
 				opt_channel_name);
+		/* Stop looping upon error */
 		if (ret < 0) {
 			fprintf(stderr, "%s: ", type->opt->symbol);
-			continue;
+			goto error;
 		} else {
 			MSG("%s context %s added to %s event in %s",
 					opt_kernel ? "kernel" : "UST", type->opt->symbol,
@@ -412,7 +419,8 @@
 					opt_channel_name ? opt_channel_name : "all channels");
 		}
 	}
-
+	/* Normalise return code upon success */
+	ret = CMD_SUCCESS;
 error:
 	lttng_destroy_handle(handle);
 
@@ -421,6 +429,7 @@
 
 /*
  * Add context to channel or event.
+ * Returns one of the CMD_* result constants.
  */
 int cmd_add_context(int argc, const char **argv)
 {
@@ -445,8 +454,10 @@
 			ret = CMD_SUCCESS;
 			goto end;
 		case OPT_TYPE:
-			//Look up the index of opt_type in ctx_opts[] first,
-			//so we don't have to free(type) on failure
+			/*
+			 * Look up the index of opt_type in ctx_opts[] first,
+			 * so we don't have to free(type) on failure
+			 */
 			index = find_ctx_type_idx(opt_type);
 			if (index < 0) {
 				ERR("Unknown context type %s", opt_type);
diff --git a/src/lib/lttng-ctl/lttng-ctl.c b/src/lib/lttng-ctl/lttng-ctl.c
index e7ebab5..ebb76ec 100644
--- a/src/lib/lttng-ctl/lttng-ctl.c
+++ b/src/lib/lttng-ctl/lttng-ctl.c
@@ -462,6 +462,7 @@
 
 /*
  * Add context to event or/and channel.
+ * Returns the size of the returned payload data or a negative error code.
  */
 int lttng_add_context(struct lttng_handle *handle,
 		struct lttng_event_context *ctx, const char *event_name,
------------------------------

Daniel U. Thibault
R & D pour la d?fense Canada - Valcartier (RDDC Valcartier) / Defence R&D Canada - Valcartier (DRDC Valcartier)
Syst?me de syst?mes (SdS) / System of Systems (SoS)
Solutions informatiques et exp?rimentations (SIE) / Computing Solutions and Experimentations (CSE)
2459 Boul. Pie XI Nord
Qu?bec, QC? G3J 1X5
CANADA
Vox : (418) 844-4000 x4245
Fax : (418) 844-4538
NAC: 918V QSDJ
Gouvernement du Canada?/ Government of Canada
<http://www.valcartier.drdc-rddc.gc.ca/>

[lttng-dev] [PATCH] lttng-tools lttng commands add_context.c : Return values of the cmd_add_context() chain of functions

[David Goulet]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

So, I've merged a couple of patch here. The changed made to calibrate.c,
add_context.c and documentation added to liblttng-ctl.

Please, for typo/comments, feel free to submit only one patch. I've no problem
with that.

On 12-01-26 04:27 PM, Thibault, Daniel wrote:
> Document and ensure the return values of the cmd_add_context() chain of functions.
> Make add_context() stop looping over lttng_add_context() upon failure (otherwise it reports success or failure depending only on the last call's result).
> Also patches lib/lttng-ctl/lttng-ctl.c
> ------------------------------
> From 9de81398942fe0adeb5288483ecb70e2573e69f0 Thu, 26 Jan 2012 16:23:05 -0500
> From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
> Date: Thu, 26 Jan 2012 16:22:54 -0500
> Subject: [PATCH] lttng-tools lttng commands add_context.c : Return values of the cmd_add_context() chain of functions
> 
> diff --git a/src/bin/lttng/commands/add_context.c b/src/bin/lttng/commands/add_context.c
> index d0f5b6f..1c63d90 100644
> --- a/src/bin/lttng/commands/add_context.c
> +++ b/src/bin/lttng/commands/add_context.c
> @@ -358,6 +358,7 @@
>  
>  /*
>   * Add context to channel or event.
> + * Returns one of the CMD_* result constants.
>   */
>  static int add_context(char *session_name)
>  {
> @@ -379,7 +380,7 @@
>  
>  	handle = lttng_create_handle(session_name, &dom);
>  	if (handle == NULL) {
> -		ret = -1;
> +		ret = CMD_ERROR;
>  		goto error;
>  	}
>  
> @@ -400,11 +401,17 @@
>  		}
>  		DBG("Adding context...");
>  
> +		/*
> +		 * lttng_add_context() returns the size
> +		 * of the returned payload data
> +		 * or a negative error code.
> +		 */

I did not had this because the API is well documented so it would duplicate
comments across the code and I prefer not since changing the API makes the
comments useless.

>  		ret = lttng_add_context(handle, &context, opt_event_name,
>  				opt_channel_name);
> +		/* Stop looping upon error */
>  		if (ret < 0) {
>  			fprintf(stderr, "%s: ", type->opt->symbol);
> -			continue;
> +			goto error;

Ok, here I am really not sure. The add-context command allows multiple "-t"
options and they are all added one by one and not in one single blob. So, if the
add context fails, you get an error on stderr and the next context is processed.
So, unless I missed something here, it's the correct behavior.

Btw, I've changed "fprintf(stderr" to ERR().

>  		} else {
>  			MSG("%s context %s added to %s event in %s",
>  					opt_kernel ? "kernel" : "UST", type->opt->symbol,
> @@ -412,7 +419,8 @@
>  					opt_channel_name ? opt_channel_name : "all channels");
>  		}
>  	}
> -
> +	/* Normalise return code upon success */
> +	ret = CMD_SUCCESS;

excellent.

I'll go over the last patches you sent soon.

Thanks!
David

>  error:
>  	lttng_destroy_handle(handle);
>  
> @@ -421,6 +429,7 @@
>  
>  /*
>   * Add context to channel or event.
> + * Returns one of the CMD_* result constants.
>   */
>  int cmd_add_context(int argc, const char **argv)
>  {
> @@ -445,8 +454,10 @@
>  			ret = CMD_SUCCESS;
>  			goto end;
>  		case OPT_TYPE:
> -			//Look up the index of opt_type in ctx_opts[] first,
> -			//so we don't have to free(type) on failure
> +			/*
> +			 * Look up the index of opt_type in ctx_opts[] first,
> +			 * so we don't have to free(type) on failure
> +			 */
>  			index = find_ctx_type_idx(opt_type);
>  			if (index < 0) {
>  				ERR("Unknown context type %s", opt_type);
> diff --git a/src/lib/lttng-ctl/lttng-ctl.c b/src/lib/lttng-ctl/lttng-ctl.c
> index e7ebab5..ebb76ec 100644
> --- a/src/lib/lttng-ctl/lttng-ctl.c
> +++ b/src/lib/lttng-ctl/lttng-ctl.c
> @@ -462,6 +462,7 @@
>  
>  /*
>   * Add context to event or/and channel.
> + * Returns the size of the returned payload data or a negative error code.
>   */
>  int lttng_add_context(struct lttng_handle *handle,
>  		struct lttng_event_context *ctx, const char *event_name,
> ------------------------------
> 
> Daniel U. Thibault
> R & D pour la d?fense Canada - Valcartier (RDDC Valcartier) / Defence R&D Canada - Valcartier (DRDC Valcartier)
> Syst?me de syst?mes (SdS) / System of Systems (SoS)
> Solutions informatiques et exp?rimentations (SIE) / Computing Solutions and Experimentations (CSE)
> 2459 Boul. Pie XI Nord
> Qu?bec, QC  G3J 1X5
> CANADA
> Vox : (418) 844-4000 x4245
> Fax : (418) 844-4538
> NAC: 918V QSDJ
> Gouvernement du Canada / Government of Canada
> <http://www.valcartier.drdc-rddc.gc.ca/>
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iQEcBAEBAgAGBQJPIsfgAAoJEELoaioR9I02krEIAKj5nPYXGLwzlPVmBYS5sdOX
jERXlUBFUNex79Db+kLK45Nas+LJ5p2J/peTX8qqIfK93Uxi8hnFDFmkd7/ffVV7
Kc41psvOO3BJqbDCmcGOIOMGcXMDkoGHGadX4ny4w6YkBEaB+TrkV0Ce6MTk97P7
b653VQho7H+BeChl3SIUneCjE3Hmcu2RjcMYGqCIcxrla4Eg07gTCnohzF6jO+oy
depTwrsR6/1giWNpIDagRzf6YfGi0ye/p+ZhkM1aPzN12HD7kgxksaiQDg5KHqtT
lnICZvlDZjCA0XeQsv/f1OkW0rukdKJuf9mve29UWhHriaMe1U7u+D36qnQkulo=
=mX0b
-----END PGP SIGNATURE-----

[lttng-dev] [PATCH] lttng-tools lttng commands add_context.c :Return values of the cmd_add_context() chain of functions

[Thibault, Daniel]

-----Message d'origine-----
De?: David Goulet [mailto:dgoulet at efficios.com] 
Envoy??: 27 janvier 2012 10:51

> > +		/*
> > +		 * lttng_add_context() returns the size
> > +		 * of the returned payload data
> > +		 * or a negative error code.
> > +		 */
>
> I did not had this because the API is well documented so it would duplicate
> comments across the code and I prefer not since changing the API makes the
> comments useless.

   As for claiming the "API is well documented", I obviously beg to differ.  :-)

   I put that comment in more to justify my code edits than just to "comment the code", so leaving it out of the patch is fine.

> >  		ret = lttng_add_context(handle, &context, opt_event_name,
> >  				opt_channel_name);
> > +		/* Stop looping upon error */
> >  		if (ret < 0) {
> >  			fprintf(stderr, "%s: ", type->opt->symbol);
> > -			continue;
> > +			goto error;
>
> Ok, here I am really not sure. The add-context command allows multiple "-t"
> options and they are all added one by one and not in one single blob. So, if the
> add context fails, you get an error on stderr and the next context is processed.
> So, unless I missed something here, it's the correct behavior.

   The problem with that behaviour is that it is not reflected in the return code of add_context().  The original code would return CMD_SUCCESS or CMD_ERROR depending on which -t option is processed last (assuming we have a -t option that fails), something I'm very uncomfortable with.

   A possible solution would be to add a new result code, say "CMD_WARNINGS", and treat the failing lttng_add_context() calls as warnings rather than errors.  Or maybe add an option "-w|--stoponwarn" so the user can decide what the behaviour should be.

Daniel U. Thibault
R & D pour la d?fense Canada - Valcartier (RDDC Valcartier) / Defence R&D Canada - Valcartier (DRDC Valcartier)
Syst?me de syst?mes (SdS) / System of Systems (SoS)
Solutions informatiques et exp?rimentations (SIE) / Computing Solutions and Experimentations (CSE)
2459 Boul. Pie XI Nord
Qu?bec, QC  G3J 1X5
CANADA
Vox : (418) 844-4000 x4245
Fax : (418) 844-4538
NAC: 918V QSDJ
Gouvernement du Canada / Government of Canada
<http://www.valcartier.drdc-rddc.gc.ca/>

[lttng-dev] [PATCH] lttng-tools lttng commands add_context.c :Return values of the cmd_add_context() chain of functions

[David Goulet]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



On 12-01-27 11:36 AM, Thibault, Daniel wrote:
> -----Message d'origine-----
> De : David Goulet [mailto:dgoulet at efficios.com] 
> Envoy? : 27 janvier 2012 10:51
> 
>>> +		/*
>>> +		 * lttng_add_context() returns the size
>>> +		 * of the returned payload data
>>> +		 * or a negative error code.
>>> +		 */
>>
>> I did not had this because the API is well documented so it would duplicate
>> comments across the code and I prefer not since changing the API makes the
>> comments useless.
> 
>    As for claiming the "API is well documented", I obviously beg to differ.  :-)

I was talking about lttng.h, the documentation is, I think, good. Any
improvement is very welcome. ;)

> 
>    I put that comment in more to justify my code edits than just to "comment the code", so leaving it out of the patch is fine.
> 
>>>  		ret = lttng_add_context(handle, &context, opt_event_name,
>>>  				opt_channel_name);
>>> +		/* Stop looping upon error */
>>>  		if (ret < 0) {
>>>  			fprintf(stderr, "%s: ", type->opt->symbol);
>>> -			continue;
>>> +			goto error;
>>
>> Ok, here I am really not sure. The add-context command allows multiple "-t"
>> options and they are all added one by one and not in one single blob. So, if the
>> add context fails, you get an error on stderr and the next context is processed.
>> So, unless I missed something here, it's the correct behavior.
> 
>    The problem with that behaviour is that it is not reflected in the return code of add_context().  The original code would return CMD_SUCCESS or CMD_ERROR depending on which -t option is processed last (assuming we have a -t option that fails), something I'm very uncomfortable with.
> 
>    A possible solution would be to add a new result code, say "CMD_WARNINGS", and treat the failing lttng_add_context() calls as warnings rather than errors.  Or maybe add an option "-w|--stoponwarn" so the user can decide what the behaviour should be.
> 

Yep, I see your point now. I agree that a CMD_WARNING could be a way to go. The
command will complete but without being a success telling the user to look in
stderr for partial problem.

I'm fine with that. Yannick, do you have any suggestion about that? For me, the
warning seems the best choice.

Cheers
David

> Daniel U. Thibault
> R & D pour la d?fense Canada - Valcartier (RDDC Valcartier) / Defence R&D Canada - Valcartier (DRDC Valcartier)
> Syst?me de syst?mes (SdS) / System of Systems (SoS)
> Solutions informatiques et exp?rimentations (SIE) / Computing Solutions and Experimentations (CSE)
> 2459 Boul. Pie XI Nord
> Qu?bec, QC  G3J 1X5
> CANADA
> Vox : (418) 844-4000 x4245
> Fax : (418) 844-4538
> NAC: 918V QSDJ
> Gouvernement du Canada / Government of Canada
> <http://www.valcartier.drdc-rddc.gc.ca/>
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iQEcBAEBAgAGBQJPItQsAAoJEELoaioR9I02C2oIALUsJeVWZQpHlB3kIV0jF2e2
Ncf4yS4nX/aNG4TFxl1UDJTG2eLId38gthKr8KBmAUp61MzX8eHYPvIYswStsyMq
qDV9xQjJ1Jm/Mh9vJfaDwM3Tv4qF3/HlfMEL3+iZzeE+0Q74ZpjZD53AQiJ5U+/J
A0jh9byn3cLWPlwbxy44MoHe5TOy3kTpc0h/Etbk0WldtBBwbGrAAiYaGA/Itdh1
8EFldCUegTh4Y8mSpwXYxCW+HivVWvuBFnP/u/jJL82E4wAN5AEJoBnKO/ybyAS6
foRSORdUFJ0ooHkjpmABoGKTW6KErDA3eLlI8TDKyqZryticaIYSpUwG1UFZOVs=
=e4jL
-----END PGP SIGNATURE-----