[lttng-dev] [PATCH] lttng-tools lib/lttng-ctl/lttng-ctl.c : Document return values and more

[lttng-dev] [PATCH] lttng-tools lib/lttng-ctl/lttng-ctl.c : Document return values and more

[Thibault, Daniel]

The lttng/commands/create.c patch improves the usage() printout and documents and enforces the return values for the cmd_create chain of calls.
The lib/lttng-ctl/lttng-ctl.c patch (the major part of this submission) documents the return values of all functions. Some points:
* lttng_list_tracepoints()'s documentation was wrong.
* check_tracing_group() had grp_id = getgroups(...); if (grp_id < -1) { ... but, according to the getgroups() documentation, the return value can never be < -1
* the snprintf() calls of set_session_daemon_path() cannot return < 0 since GNU C 2.1; the ifs were rewritten to handle either case
* ask_sessiond() still does not compare the number of bytes received with the number requested.  What should it do if they don't match?  Throw everything away and report an error?
* set_session_daemon_path() was rewritten to reduce redundant segments of code
------------------------------
From b200f661f1b17c6fb63147e0ad9304cf462b84e6 Fri, 27 Jan 2012 13:29:29 -0500
From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
Date: Fri, 27 Jan 2012 13:29:14 -0500
Subject: [PATCH] lttng-tools lib/lttng-ctl/lttng-ctl.c : Document return values and more

diff --git a/src/bin/lttng/commands/create.c b/src/bin/lttng/commands/create.c
index 34754c7..94ff634 100644
--- a/src/bin/lttng/commands/create.c
+++ b/src/bin/lttng/commands/create.c
@@ -52,8 +52,9 @@
 {
 	fprintf(ofp, "usage: lttng create [options] [NAME]\n");
 	fprintf(ofp, "\n");
+	fprintf(ofp, "  The default NAME is 'auto-yyyymmdd-hhmmss'\n");
 	fprintf(ofp, "  -h, --help           Show this help\n");
-	fprintf(ofp, "      --list-options       Simple listing of options\n");
+	fprintf(ofp, "      --list-options   Simple listing of options\n");
 	fprintf(ofp, "  -o, --output PATH    Specify output path for traces\n");
 	fprintf(ofp, "\n");
 }
@@ -61,12 +62,13 @@
 /*
  *  create_session
  *
- *  Create a tracing session. If no name specified, a default name will be
- *  generated.
+ *  Create a tracing session.
+ *  If no name is specified, a default name is generated.
+ *  Returns one of the CMD_* result constants.
  */
 static int create_session()
 {
-	int ret, have_name = 0;
+	int ret;
 	char datetime[16];
 	char *session_name, *traces_path = NULL, *alloc_path = NULL;
 	time_t rawtime;
@@ -79,37 +81,33 @@
 
 	/* Auto session name creation */
 	if (opt_session_name == NULL) {
-		ret = asprintf(&session_name, "auto-%s", datetime);
+		ret = asprintf(&session_name, "auto");
 		if (ret < 0) {
 			perror("asprintf session name");
+			ret = CMD_ERROR;
 			goto error;
 		}
 		DBG("Auto session name set to %s", session_name);
 	} else {
 		session_name = opt_session_name;
-		have_name = 1;
 	}
 
 	/* Auto output path */
 	if (opt_output_path == NULL) {
 		alloc_path = strdup(config_get_default_path());
 		if (alloc_path == NULL) {
-			ERR("Home path not found.\n \
-				 Please specify an output path using -o, --output PATH");
+			ERR("Home path not found.\n%s"
+				"Please specify an output path using -o, --output PATH\n");
 			ret = CMD_FATAL;
 			goto error;
 		}
 
-		if (have_name) {
-			ret = asprintf(&traces_path, "%s/" DEFAULT_TRACE_DIR_NAME
+		ret = asprintf(&traces_path, "%s/" DEFAULT_TRACE_DIR_NAME
 					"/%s-%s", alloc_path, session_name, datetime);
-		} else {
-			ret = asprintf(&traces_path, "%s/" DEFAULT_TRACE_DIR_NAME
-					"/%s", alloc_path, session_name);
-		}
 
 		if (ret < 0) {
 			perror("asprintf trace dir name");
+			ret = CMD_ERROR;
 			goto error;
 		}
 	} else {
@@ -150,6 +148,7 @@
  *  cmd_create
  *
  *  The 'create <options>' first level command
+ *  Returns one of the CMD_* result constants.
  */
 int cmd_create(int argc, const char **argv)
 {
diff --git a/src/lib/lttng-ctl/lttng-ctl.c b/src/lib/lttng-ctl/lttng-ctl.c
index ebb76ec..aeef7f0 100644
--- a/src/lib/lttng-ctl/lttng-ctl.c
+++ b/src/lib/lttng-ctl/lttng-ctl.c
@@ -83,8 +83,8 @@
 /*
  * Send lttcomm_session_msg to the session daemon.
  *
- * On success, return 0
- * On error, return error code
+ * On success, returns the number of bytes sent (>=0)
+ * On error, returns -1
  */
 static int send_session_msg(struct lttcomm_session_msg *lsm)
 {
@@ -105,8 +105,8 @@
 /*
  * Receive data from the sessiond socket.
  *
- * On success, return 0
- * On error, return recv() error code
+ * On success, returns the number of bytes received (>=0)
+ * On error, returns -1 (recvmsg() error) or -ENOTCONN
  */
 static int recv_data_sessiond(void *buf, size_t len)
 {
@@ -124,7 +124,7 @@
 }
 
 /*
- *  Check if the specified group name exist.
+ *  Check if we are in the specified group.
  *
  *  If yes return 1, else return -1.
  */
@@ -153,13 +153,15 @@
 	}
 
 	/* Alloc group list of the right size */
+    /* Note that malloc(0) will return a valid pointer */
 	grp_list = malloc(grp_list_size * sizeof(gid_t));
 	if (!grp_list) {
-		ret = -1;
+		perror("malloc");
 		goto end;
 	}
+
 	grp_id = getgroups(grp_list_size, grp_list);
-	if (grp_id < -1) {
+	if (grp_id < 0) {
 		perror("getgroups");
 		goto free_list;
 	}
@@ -209,8 +211,10 @@
 }
 
 /*
- * Set sessiond socket path by putting it in the global sessiond_sock_path
- * variable.
+ * Set sessiond socket path by putting it in the global
+ * sessiond_sock_path variable.
+ * Returns 0 on success,
+ * -ENOMEM on failure (the sessiond socket path is somehow too long)
  */
 static int set_session_daemon_path(void)
 {
@@ -225,35 +229,31 @@
 		in_tgroup = check_tracing_group(tracing_group);
 	}
 
-	if (uid == 0) {
-		/* Root */
+	if ((uid == 0) || in_tgroup) {
 		copy_string(sessiond_sock_path,
 				DEFAULT_GLOBAL_CLIENT_UNIX_SOCK,
 				sizeof(sessiond_sock_path));
-	} else if (in_tgroup) {
-		/* Tracing group */
-		copy_string(sessiond_sock_path,
-				DEFAULT_GLOBAL_CLIENT_UNIX_SOCK,
-				sizeof(sessiond_sock_path));
-
-		ret = try_connect_sessiond(sessiond_sock_path);
-		if (ret < 0) {
-			/* Global session daemon not available */
-			if (snprintf(sessiond_sock_path, sizeof(sessiond_sock_path),
-						DEFAULT_HOME_CLIENT_UNIX_SOCK,
-						getenv("HOME")) < 0) {
-				return -ENOMEM;
-			}
+	}
+	if (uid != 0) {
+		if (in_tgroup) {
+			/* Tracing group */
+			ret = try_connect_sessiond(sessiond_sock_path);
+			if (ret >= 0) goto end;
+			/* Global session daemon not available... */
 		}
-	} else {
-		/* Not in tracing group and not root, default */
-		if (snprintf(sessiond_sock_path, PATH_MAX,
-					DEFAULT_HOME_CLIENT_UNIX_SOCK,
-					getenv("HOME")) < 0) {
+		/* ...or not in tracing group (and not root), default */
+		/*
+		 * With GNU C <  2.1, snprintf returns -1 if the target buffer is too small;
+		 * With GNU C >= 2.1, snprintf returns the required size (excluding closing null)
+		 */
+		ret = snprintf(sessiond_sock_path, sizeof(sessiond_sock_path),
+				DEFAULT_HOME_CLIENT_UNIX_SOCK,
+				getenv("HOME"));
+		if ((ret < 0) || (ret >= sizeof(sessiond_sock_path))) {
 			return -ENOMEM;
 		}
 	}
-
+end:
 	return 0;
 }
 
@@ -268,7 +268,7 @@
 
 	ret = set_session_daemon_path();
 	if (ret < 0) {
-		return ret;
+		return -1; /* set_session_daemon_path() returns -ENOMEM */
 	}
 
 	/* Connect to the sesssion daemon */
@@ -284,7 +284,8 @@
 }
 
 /*
- *  Clean disconnect the session daemon.
+ *  Clean disconnect from the session daemon.
+ *  On success, return 0. On error, return -1.
  */
 static int disconnect_sessiond(void)
 {
@@ -373,6 +374,7 @@
 
 /*
  * Create lttng handle and return pointer.
+ * The returned pointer will be NULL in case of malloc() error.
  */
 struct lttng_handle *lttng_create_handle(const char *session_name,
 		struct lttng_domain *domain)
@@ -408,6 +410,7 @@
 
 /*
  * Register an outside consumer.
+ * Returns size of returned session payload data or a negative error code.
  */
 int lttng_register_consumer(struct lttng_handle *handle,
 		const char *socket_path)
@@ -425,7 +428,8 @@
 }
 
 /*
- *  Start tracing for all trace of the session.
+ *  Start tracing for all traces of the session.
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_start_tracing(const char *session_name)
 {
@@ -443,7 +447,8 @@
 }
 
 /*
- *  Stop tracing for all trace of the session.
+ *  Stop tracing for all traces of the session.
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_stop_tracing(const char *session_name)
 {
@@ -495,7 +500,10 @@
 }
 
 /*
- * Enable event
+ *  Enable event(s) for a channel.
+ *  If no event name is specified, all events are enabled.
+ *  If no channel name is specified, the default 'channel0' is used.
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_enable_event(struct lttng_handle *handle,
 		struct lttng_event *ev, const char *channel_name)
@@ -507,13 +515,9 @@
 	}
 
 	/* If no channel name, we put the default name */
-	if (channel_name == NULL) {
-		copy_string(lsm.u.enable.channel_name, DEFAULT_CHANNEL_NAME,
-				sizeof(lsm.u.enable.channel_name));
-	} else {
-		copy_string(lsm.u.enable.channel_name, channel_name,
-				sizeof(lsm.u.enable.channel_name));
-	}
+	copy_string(lsm.u.enable.channel_name,
+			channel_name ? channel_name : DEFAULT_CHANNEL_NAME,
+			sizeof(lsm.u.enable.channel_name));
 
 	copy_lttng_domain(&lsm.domain, &handle->domain);
 
@@ -531,7 +535,10 @@
 }
 
 /*
- * Disable event of a channel and domain.
+ *  Disable event(s) of a channel and domain.
+ *  If no event name is specified, all events are disabled.
+ *  If no channel name is specified, the default 'channel0' is used.
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_disable_event(struct lttng_handle *handle, const char *name,
 		const char *channel_name)
@@ -542,13 +549,9 @@
 		return -1;
 	}
 
-	if (channel_name) {
-		copy_string(lsm.u.disable.channel_name, channel_name,
-				sizeof(lsm.u.disable.channel_name));
-	} else {
-		copy_string(lsm.u.disable.channel_name, DEFAULT_CHANNEL_NAME,
-				sizeof(lsm.u.disable.channel_name));
-	}
+	copy_string(lsm.u.disable.channel_name,
+			channel_name ? channel_name : DEFAULT_CHANNEL_NAME,
+			sizeof(lsm.u.disable.channel_name));
 
 	copy_lttng_domain(&lsm.domain, &handle->domain);
 
@@ -566,7 +569,8 @@
 }
 
 /*
- * Enable channel per domain
+ *  Enable channel per domain
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_enable_channel(struct lttng_handle *handle,
 		struct lttng_channel *chan)
@@ -593,7 +597,8 @@
 }
 
 /*
- * All tracing will be stopped for registered events of the channel.
+ *  All tracing will be stopped for registered events of the channel.
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_disable_channel(struct lttng_handle *handle, const char *name)
 {
@@ -618,10 +623,10 @@
 }
 
 /*
- * List all available tracepoints of domain.
- *
- * Return the size (bytes) of the list and set the events array.
- * On error, return negative value.
+ *  Lists all available tracepoints of domain.
+ *  Sets the contents of the events array.
+ *  Returns the number of lttng_event entries in events;
+ *  on error, returns a negative value.
  */
 int lttng_list_tracepoints(struct lttng_handle *handle,
 		struct lttng_event **events)
@@ -645,10 +650,12 @@
 }
 
 /*
- *  Return a human readable string of code
+ *  Returns a human readable string describing
+ *  the error code (a negative value).
  */
 const char *lttng_strerror(int code)
 {
+	/* lttcomm error codes range from -LTTCOMM_OK down to -LTTCOMM_NR */
 	if (code > -LTTCOMM_OK) {
 		return "Ended with errors";
 	}
@@ -657,7 +664,8 @@
 }
 
 /*
- *  Create a brand new session using name.
+ *  Create a brand new session using name and path.
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_create_session(const char *name, const char *path)
 {
@@ -672,6 +680,7 @@
 
 /*
  *  Destroy session using name.
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_destroy_session(const char *session_name)
 {
@@ -690,9 +699,9 @@
 
 /*
  *  Ask the session daemon for all available sessions.
- *
- *  Return number of session.
- *  On error, return negative value.
+ *  Sets the contents of the sessions array.
+ *  Returns the number of lttng_session entries in sessions;
+ *  on error, returns a negative value.
  */
 int lttng_list_sessions(struct lttng_session **sessions)
 {
@@ -709,7 +718,10 @@
 }
 
 /*
- * List domain of a session.
+ *  Ask the session daemon for all available domains of a session.
+ *  Sets the contents of the domains array.
+ *  Returns the number of lttng_domain entries in domains;
+ *  on error, returns a negative value.
  */
 int lttng_list_domains(const char *session_name,
 		struct lttng_domain **domains)
@@ -734,7 +746,10 @@
 }
 
 /*
- * List channels of a session
+ *  Ask the session daemon for all available channels of a session.
+ *  Sets the contents of the channels array.
+ *  Returns the number of lttng_channel entries in channels;
+ *  on error, returns a negative value.
  */
 int lttng_list_channels(struct lttng_handle *handle,
 		struct lttng_channel **channels)
@@ -761,7 +776,10 @@
 }
 
 /*
- * List events of a session channel.
+ *  Ask the session daemon for all available events of a session channel.
+ *  Sets the contents of the events array.
+ *  Returns the number of lttng_event entries in events;
+ *  on error, returns a negative value.
  */
 int lttng_list_events(struct lttng_handle *handle,
 		const char *channel_name, struct lttng_event **events)
@@ -791,8 +809,9 @@
 }
 
 /*
- * Set tracing group variable with name. This function allocate memory pointed
- * by tracing_group.
+ * Sets the tracing_group variable with name.
+ * This function allocates memory pointed to by tracing_group.
+ * On success, returns 0, on error, returns -1 (null name) or -ENOMEM.
  */
 int lttng_set_tracing_group(const char *name)
 {
@@ -831,6 +850,7 @@
 
 /*
  * Set default channel attributes.
+ * If either or both of the arguments are null, nothing happens.
  */
 void lttng_channel_set_default_attr(struct lttng_domain *domain,
 		struct lttng_channel_attr *attr)
@@ -875,7 +895,7 @@
  * Check if session daemon is alive.
  *
  * Return 1 if alive or 0 if not.
- * On error return -1
+ * On error returns a negative value.
  */
 int lttng_session_daemon_alive(void)
 {
------------------------------

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 lib/lttng-ctl/lttng-ctl.c : Document return values and more

[David Goulet]

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

Hi Daniel,

Reviewed this and it's all very good except some minor minor details.

However, there is a lot of comments added along with important modifications to
two functions.

Can you please make three different patch for that (ideally with git
format-pacth :)). Especially for the modifications in liblttng-ctl which is
quite a core function.

It's very important to separate those kind of changes in order for us to easily
"git bisect/blame" and pin point commit/committer if anything goes wrong with
the code.

Thanks a lot!
David

On 12-01-27 01:41 PM, Thibault, Daniel wrote:
> The lttng/commands/create.c patch improves the usage() printout and documents and enforces the return values for the cmd_create chain of calls.
> The lib/lttng-ctl/lttng-ctl.c patch (the major part of this submission) documents the return values of all functions. Some points:
> * lttng_list_tracepoints()'s documentation was wrong.
> * check_tracing_group() had grp_id = getgroups(...); if (grp_id < -1) { ... but, according to the getgroups() documentation, the return value can never be < -1
> * the snprintf() calls of set_session_daemon_path() cannot return < 0 since GNU C 2.1; the ifs were rewritten to handle either case
> * ask_sessiond() still does not compare the number of bytes received with the number requested.  What should it do if they don't match?  Throw everything away and report an error?
> * set_session_daemon_path() was rewritten to reduce redundant segments of code
> ------------------------------
> From b200f661f1b17c6fb63147e0ad9304cf462b84e6 Fri, 27 Jan 2012 13:29:29 -0500
> From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
> Date: Fri, 27 Jan 2012 13:29:14 -0500
> Subject: [PATCH] lttng-tools lib/lttng-ctl/lttng-ctl.c : Document return values and more
> 
> diff --git a/src/bin/lttng/commands/create.c b/src/bin/lttng/commands/create.c
> index 34754c7..94ff634 100644
> --- a/src/bin/lttng/commands/create.c
> +++ b/src/bin/lttng/commands/create.c
> @@ -52,8 +52,9 @@
>  {
>  	fprintf(ofp, "usage: lttng create [options] [NAME]\n");
>  	fprintf(ofp, "\n");
> +	fprintf(ofp, "  The default NAME is 'auto-yyyymmdd-hhmmss'\n");
>  	fprintf(ofp, "  -h, --help           Show this help\n");
> -	fprintf(ofp, "      --list-options       Simple listing of options\n");
> +	fprintf(ofp, "      --list-options   Simple listing of options\n");
>  	fprintf(ofp, "  -o, --output PATH    Specify output path for traces\n");
>  	fprintf(ofp, "\n");
>  }
> @@ -61,12 +62,13 @@
>  /*
>   *  create_session
>   *
> - *  Create a tracing session. If no name specified, a default name will be
> - *  generated.
> + *  Create a tracing session.
> + *  If no name is specified, a default name is generated.
> + *  Returns one of the CMD_* result constants.
>   */
>  static int create_session()
>  {
> -	int ret, have_name = 0;
> +	int ret;
>  	char datetime[16];
>  	char *session_name, *traces_path = NULL, *alloc_path = NULL;
>  	time_t rawtime;
> @@ -79,37 +81,33 @@
>  
>  	/* Auto session name creation */
>  	if (opt_session_name == NULL) {
> -		ret = asprintf(&session_name, "auto-%s", datetime);
> +		ret = asprintf(&session_name, "auto");
>  		if (ret < 0) {
>  			perror("asprintf session name");
> +			ret = CMD_ERROR;
>  			goto error;
>  		}
>  		DBG("Auto session name set to %s", session_name);
>  	} else {
>  		session_name = opt_session_name;
> -		have_name = 1;
>  	}
>  
>  	/* Auto output path */
>  	if (opt_output_path == NULL) {
>  		alloc_path = strdup(config_get_default_path());
>  		if (alloc_path == NULL) {
> -			ERR("Home path not found.\n \
> -				 Please specify an output path using -o, --output PATH");
> +			ERR("Home path not found.\n%s"
> +				"Please specify an output path using -o, --output PATH\n");
>  			ret = CMD_FATAL;
>  			goto error;
>  		}
>  
> -		if (have_name) {
> -			ret = asprintf(&traces_path, "%s/" DEFAULT_TRACE_DIR_NAME
> +		ret = asprintf(&traces_path, "%s/" DEFAULT_TRACE_DIR_NAME
>  					"/%s-%s", alloc_path, session_name, datetime);
> -		} else {
> -			ret = asprintf(&traces_path, "%s/" DEFAULT_TRACE_DIR_NAME
> -					"/%s", alloc_path, session_name);
> -		}
>  
>  		if (ret < 0) {
>  			perror("asprintf trace dir name");
> +			ret = CMD_ERROR;
>  			goto error;
>  		}
>  	} else {
> @@ -150,6 +148,7 @@
>   *  cmd_create
>   *
>   *  The 'create <options>' first level command
> + *  Returns one of the CMD_* result constants.
>   */
>  int cmd_create(int argc, const char **argv)
>  {
> diff --git a/src/lib/lttng-ctl/lttng-ctl.c b/src/lib/lttng-ctl/lttng-ctl.c
> index ebb76ec..aeef7f0 100644
> --- a/src/lib/lttng-ctl/lttng-ctl.c
> +++ b/src/lib/lttng-ctl/lttng-ctl.c
> @@ -83,8 +83,8 @@
>  /*
>   * Send lttcomm_session_msg to the session daemon.
>   *
> - * On success, return 0
> - * On error, return error code
> + * On success, returns the number of bytes sent (>=0)
> + * On error, returns -1
>   */
>  static int send_session_msg(struct lttcomm_session_msg *lsm)
>  {
> @@ -105,8 +105,8 @@
>  /*
>   * Receive data from the sessiond socket.
>   *
> - * On success, return 0
> - * On error, return recv() error code
> + * On success, returns the number of bytes received (>=0)
> + * On error, returns -1 (recvmsg() error) or -ENOTCONN
>   */
>  static int recv_data_sessiond(void *buf, size_t len)
>  {
> @@ -124,7 +124,7 @@
>  }
>  
>  /*
> - *  Check if the specified group name exist.
> + *  Check if we are in the specified group.
>   *
>   *  If yes return 1, else return -1.
>   */
> @@ -153,13 +153,15 @@
>  	}
>  
>  	/* Alloc group list of the right size */
> +    /* Note that malloc(0) will return a valid pointer */
>  	grp_list = malloc(grp_list_size * sizeof(gid_t));
>  	if (!grp_list) {
> -		ret = -1;
> +		perror("malloc");
>  		goto end;
>  	}
> +
>  	grp_id = getgroups(grp_list_size, grp_list);
> -	if (grp_id < -1) {
> +	if (grp_id < 0) {
>  		perror("getgroups");
>  		goto free_list;
>  	}
> @@ -209,8 +211,10 @@
>  }
>  
>  /*
> - * Set sessiond socket path by putting it in the global sessiond_sock_path
> - * variable.
> + * Set sessiond socket path by putting it in the global
> + * sessiond_sock_path variable.
> + * Returns 0 on success,
> + * -ENOMEM on failure (the sessiond socket path is somehow too long)
>   */
>  static int set_session_daemon_path(void)
>  {
> @@ -225,35 +229,31 @@
>  		in_tgroup = check_tracing_group(tracing_group);
>  	}
>  
> -	if (uid == 0) {
> -		/* Root */
> +	if ((uid == 0) || in_tgroup) {
>  		copy_string(sessiond_sock_path,
>  				DEFAULT_GLOBAL_CLIENT_UNIX_SOCK,
>  				sizeof(sessiond_sock_path));
> -	} else if (in_tgroup) {
> -		/* Tracing group */
> -		copy_string(sessiond_sock_path,
> -				DEFAULT_GLOBAL_CLIENT_UNIX_SOCK,
> -				sizeof(sessiond_sock_path));
> -
> -		ret = try_connect_sessiond(sessiond_sock_path);
> -		if (ret < 0) {
> -			/* Global session daemon not available */
> -			if (snprintf(sessiond_sock_path, sizeof(sessiond_sock_path),
> -						DEFAULT_HOME_CLIENT_UNIX_SOCK,
> -						getenv("HOME")) < 0) {
> -				return -ENOMEM;
> -			}
> +	}
> +	if (uid != 0) {
> +		if (in_tgroup) {
> +			/* Tracing group */
> +			ret = try_connect_sessiond(sessiond_sock_path);
> +			if (ret >= 0) goto end;
> +			/* Global session daemon not available... */
>  		}
> -	} else {
> -		/* Not in tracing group and not root, default */
> -		if (snprintf(sessiond_sock_path, PATH_MAX,
> -					DEFAULT_HOME_CLIENT_UNIX_SOCK,
> -					getenv("HOME")) < 0) {
> +		/* ...or not in tracing group (and not root), default */
> +		/*
> +		 * With GNU C <  2.1, snprintf returns -1 if the target buffer is too small;
> +		 * With GNU C >= 2.1, snprintf returns the required size (excluding closing null)
> +		 */
> +		ret = snprintf(sessiond_sock_path, sizeof(sessiond_sock_path),
> +				DEFAULT_HOME_CLIENT_UNIX_SOCK,
> +				getenv("HOME"));
> +		if ((ret < 0) || (ret >= sizeof(sessiond_sock_path))) {
>  			return -ENOMEM;
>  		}
>  	}
> -
> +end:
>  	return 0;
>  }
>  
> @@ -268,7 +268,7 @@
>  
>  	ret = set_session_daemon_path();
>  	if (ret < 0) {
> -		return ret;
> +		return -1; /* set_session_daemon_path() returns -ENOMEM */
>  	}
>  
>  	/* Connect to the sesssion daemon */
> @@ -284,7 +284,8 @@
>  }
>  
>  /*
> - *  Clean disconnect the session daemon.
> + *  Clean disconnect from the session daemon.
> + *  On success, return 0. On error, return -1.
>   */
>  static int disconnect_sessiond(void)
>  {
> @@ -373,6 +374,7 @@
>  
>  /*
>   * Create lttng handle and return pointer.
> + * The returned pointer will be NULL in case of malloc() error.
>   */
>  struct lttng_handle *lttng_create_handle(const char *session_name,
>  		struct lttng_domain *domain)
> @@ -408,6 +410,7 @@
>  
>  /*
>   * Register an outside consumer.
> + * Returns size of returned session payload data or a negative error code.
>   */
>  int lttng_register_consumer(struct lttng_handle *handle,
>  		const char *socket_path)
> @@ -425,7 +428,8 @@
>  }
>  
>  /*
> - *  Start tracing for all trace of the session.
> + *  Start tracing for all traces of the session.
> + *  Returns size of returned session payload data or a negative error code.
>   */
>  int lttng_start_tracing(const char *session_name)
>  {
> @@ -443,7 +447,8 @@
>  }
>  
>  /*
> - *  Stop tracing for all trace of the session.
> + *  Stop tracing for all traces of the session.
> + *  Returns size of returned session payload data or a negative error code.
>   */
>  int lttng_stop_tracing(const char *session_name)
>  {
> @@ -495,7 +500,10 @@
>  }
>  
>  /*
> - * Enable event
> + *  Enable event(s) for a channel.
> + *  If no event name is specified, all events are enabled.
> + *  If no channel name is specified, the default 'channel0' is used.
> + *  Returns size of returned session payload data or a negative error code.
>   */
>  int lttng_enable_event(struct lttng_handle *handle,
>  		struct lttng_event *ev, const char *channel_name)
> @@ -507,13 +515,9 @@
>  	}
>  
>  	/* If no channel name, we put the default name */
> -	if (channel_name == NULL) {
> -		copy_string(lsm.u.enable.channel_name, DEFAULT_CHANNEL_NAME,
> -				sizeof(lsm.u.enable.channel_name));
> -	} else {
> -		copy_string(lsm.u.enable.channel_name, channel_name,
> -				sizeof(lsm.u.enable.channel_name));
> -	}
> +	copy_string(lsm.u.enable.channel_name,
> +			channel_name ? channel_name : DEFAULT_CHANNEL_NAME,
> +			sizeof(lsm.u.enable.channel_name));
>  
>  	copy_lttng_domain(&lsm.domain, &handle->domain);
>  
> @@ -531,7 +535,10 @@
>  }
>  
>  /*
> - * Disable event of a channel and domain.
> + *  Disable event(s) of a channel and domain.
> + *  If no event name is specified, all events are disabled.
> + *  If no channel name is specified, the default 'channel0' is used.
> + *  Returns size of returned session payload data or a negative error code.
>   */
>  int lttng_disable_event(struct lttng_handle *handle, const char *name,
>  		const char *channel_name)
> @@ -542,13 +549,9 @@
>  		return -1;
>  	}
>  
> -	if (channel_name) {
> -		copy_string(lsm.u.disable.channel_name, channel_name,
> -				sizeof(lsm.u.disable.channel_name));
> -	} else {
> -		copy_string(lsm.u.disable.channel_name, DEFAULT_CHANNEL_NAME,
> -				sizeof(lsm.u.disable.channel_name));
> -	}
> +	copy_string(lsm.u.disable.channel_name,
> +			channel_name ? channel_name : DEFAULT_CHANNEL_NAME,
> +			sizeof(lsm.u.disable.channel_name));
>  
>  	copy_lttng_domain(&lsm.domain, &handle->domain);
>  
> @@ -566,7 +569,8 @@
>  }
>  
>  /*
> - * Enable channel per domain
> + *  Enable channel per domain
> + *  Returns size of returned session payload data or a negative error code.
>   */
>  int lttng_enable_channel(struct lttng_handle *handle,
>  		struct lttng_channel *chan)
> @@ -593,7 +597,8 @@
>  }
>  
>  /*
> - * All tracing will be stopped for registered events of the channel.
> + *  All tracing will be stopped for registered events of the channel.
> + *  Returns size of returned session payload data or a negative error code.
>   */
>  int lttng_disable_channel(struct lttng_handle *handle, const char *name)
>  {
> @@ -618,10 +623,10 @@
>  }
>  
>  /*
> - * List all available tracepoints of domain.
> - *
> - * Return the size (bytes) of the list and set the events array.
> - * On error, return negative value.
> + *  Lists all available tracepoints of domain.
> + *  Sets the contents of the events array.
> + *  Returns the number of lttng_event entries in events;
> + *  on error, returns a negative value.
>   */
>  int lttng_list_tracepoints(struct lttng_handle *handle,
>  		struct lttng_event **events)
> @@ -645,10 +650,12 @@
>  }
>  
>  /*
> - *  Return a human readable string of code
> + *  Returns a human readable string describing
> + *  the error code (a negative value).
>   */
>  const char *lttng_strerror(int code)
>  {
> +	/* lttcomm error codes range from -LTTCOMM_OK down to -LTTCOMM_NR */
>  	if (code > -LTTCOMM_OK) {
>  		return "Ended with errors";
>  	}
> @@ -657,7 +664,8 @@
>  }
>  
>  /*
> - *  Create a brand new session using name.
> + *  Create a brand new session using name and path.
> + *  Returns size of returned session payload data or a negative error code.
>   */
>  int lttng_create_session(const char *name, const char *path)
>  {
> @@ -672,6 +680,7 @@
>  
>  /*
>   *  Destroy session using name.
> + *  Returns size of returned session payload data or a negative error code.
>   */
>  int lttng_destroy_session(const char *session_name)
>  {
> @@ -690,9 +699,9 @@
>  
>  /*
>   *  Ask the session daemon for all available sessions.
> - *
> - *  Return number of session.
> - *  On error, return negative value.
> + *  Sets the contents of the sessions array.
> + *  Returns the number of lttng_session entries in sessions;
> + *  on error, returns a negative value.
>   */
>  int lttng_list_sessions(struct lttng_session **sessions)
>  {
> @@ -709,7 +718,10 @@
>  }
>  
>  /*
> - * List domain of a session.
> + *  Ask the session daemon for all available domains of a session.
> + *  Sets the contents of the domains array.
> + *  Returns the number of lttng_domain entries in domains;
> + *  on error, returns a negative value.
>   */
>  int lttng_list_domains(const char *session_name,
>  		struct lttng_domain **domains)
> @@ -734,7 +746,10 @@
>  }
>  
>  /*
> - * List channels of a session
> + *  Ask the session daemon for all available channels of a session.
> + *  Sets the contents of the channels array.
> + *  Returns the number of lttng_channel entries in channels;
> + *  on error, returns a negative value.
>   */
>  int lttng_list_channels(struct lttng_handle *handle,
>  		struct lttng_channel **channels)
> @@ -761,7 +776,10 @@
>  }
>  
>  /*
> - * List events of a session channel.
> + *  Ask the session daemon for all available events of a session channel.
> + *  Sets the contents of the events array.
> + *  Returns the number of lttng_event entries in events;
> + *  on error, returns a negative value.
>   */
>  int lttng_list_events(struct lttng_handle *handle,
>  		const char *channel_name, struct lttng_event **events)
> @@ -791,8 +809,9 @@
>  }
>  
>  /*
> - * Set tracing group variable with name. This function allocate memory pointed
> - * by tracing_group.
> + * Sets the tracing_group variable with name.
> + * This function allocates memory pointed to by tracing_group.
> + * On success, returns 0, on error, returns -1 (null name) or -ENOMEM.
>   */
>  int lttng_set_tracing_group(const char *name)
>  {
> @@ -831,6 +850,7 @@
>  
>  /*
>   * Set default channel attributes.
> + * If either or both of the arguments are null, nothing happens.
>   */
>  void lttng_channel_set_default_attr(struct lttng_domain *domain,
>  		struct lttng_channel_attr *attr)
> @@ -875,7 +895,7 @@
>   * Check if session daemon is alive.
>   *
>   * Return 1 if alive or 0 if not.
> - * On error return -1
> + * On error returns a negative value.
>   */
>  int lttng_session_daemon_alive(void)
>  {
> ------------------------------
> 
> 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)

iQEcBAEBAgAGBQJPIv8sAAoJEELoaioR9I02fz8IAJvkCCVSW8PceqGEhHJOfGOV
WKM+TZMHW/BNN440eg9TfSaO2jevVKfTE5ej+kAJqjwPawe5vdX/YVK08reOZWxH
92OJhpMGwEOqC4LfbS+6IcwUVVPnOJO67k5H1JtFD5rOhOIYM2mu742A0s7CHQH2
cSKOsO+TDUx4LdSpsJWHakDbP/jWdtyCliFRwGmBnWhD2ZWOt2tHho47JSaVImoJ
hTU1TvR10D3e8nZTAc2atf/Ej28qzjrk/dv9YObeV4eDTFXy2uSga7RI/bArq4IT
iaIxItsuoI2kDxtL8SNYeXAtOYCtJJFjaVhJqdXKgjTE2rU/dCf1ktSFzAPdE3Q=
=ueuk
-----END PGP SIGNATURE-----

[lttng-dev] [PATCH] [RESUBMISSION] lttng-tools lib/lttng-ctl/lttng-ctl.c : Document return values and more

[Thibault, Daniel]

> However, there is a lot of comments added along with important modifications to two functions.
>
> Can you please make three different patches for that (ideally with git format-patch :)). Especially for the
> modifications in liblttng-ctl which is quite a core function.
>
> It's very important to separate those kind of changes in order for us to easily "git bisect/blame" and pin
> point commit/committer if anything goes wrong with the code.

   All right, backing up to my commit 9de81398942fe0adeb5288483ecb70e2573e69f0 and patching in three steps from there, here's a first patch that affects just lttng-tools bin/lttng/commands/create.c :

* Improve usage() output
* Document and enforce return values
* Simplify create_session() name handling

   The next patch is below this one.
------------------------------
From b58d8e97280d3cd87ae187bb79c4800f39a31f64 Fri, 27 Jan 2012 15:09:58 -0500
From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
Date: Fri, 27 Jan 2012 15:09:35 -0500
Subject: [PATCH] lttng-tools bin/lttng/commands/create.c : Improve usage(), document and enforce return values, simplify create_session() name handling

diff --git a/src/bin/lttng/commands/create.c b/src/bin/lttng/commands/create.c
index 34754c7..94ff634 100644
--- a/src/bin/lttng/commands/create.c
+++ b/src/bin/lttng/commands/create.c
@@ -52,8 +52,9 @@
 {
 	fprintf(ofp, "usage: lttng create [options] [NAME]\n");
 	fprintf(ofp, "\n");
+	fprintf(ofp, "  The default NAME is 'auto-yyyymmdd-hhmmss'\n");
 	fprintf(ofp, "  -h, --help           Show this help\n");
-	fprintf(ofp, "      --list-options       Simple listing of options\n");
+	fprintf(ofp, "      --list-options   Simple listing of options\n");
 	fprintf(ofp, "  -o, --output PATH    Specify output path for traces\n");
 	fprintf(ofp, "\n");
 }
@@ -61,12 +62,13 @@
 /*
  *  create_session
  *
- *  Create a tracing session. If no name specified, a default name will be
- *  generated.
+ *  Create a tracing session.
+ *  If no name is specified, a default name is generated.
+ *  Returns one of the CMD_* result constants.
  */
 static int create_session()
 {
-	int ret, have_name = 0;
+	int ret;
 	char datetime[16];
 	char *session_name, *traces_path = NULL, *alloc_path = NULL;
 	time_t rawtime;
@@ -79,37 +81,33 @@
 
 	/* Auto session name creation */
 	if (opt_session_name == NULL) {
-		ret = asprintf(&session_name, "auto-%s", datetime);
+		ret = asprintf(&session_name, "auto");
 		if (ret < 0) {
 			perror("asprintf session name");
+			ret = CMD_ERROR;
 			goto error;
 		}
 		DBG("Auto session name set to %s", session_name);
 	} else {
 		session_name = opt_session_name;
-		have_name = 1;
 	}
 
 	/* Auto output path */
 	if (opt_output_path == NULL) {
 		alloc_path = strdup(config_get_default_path());
 		if (alloc_path == NULL) {
-			ERR("Home path not found.\n \
-				 Please specify an output path using -o, --output PATH");
+			ERR("Home path not found.\n%s"
+				"Please specify an output path using -o, --output PATH\n");
 			ret = CMD_FATAL;
 			goto error;
 		}
 
-		if (have_name) {
-			ret = asprintf(&traces_path, "%s/" DEFAULT_TRACE_DIR_NAME
+		ret = asprintf(&traces_path, "%s/" DEFAULT_TRACE_DIR_NAME
 					"/%s-%s", alloc_path, session_name, datetime);
-		} else {
-			ret = asprintf(&traces_path, "%s/" DEFAULT_TRACE_DIR_NAME
-					"/%s", alloc_path, session_name);
-		}
 
 		if (ret < 0) {
 			perror("asprintf trace dir name");
+			ret = CMD_ERROR;
 			goto error;
 		}
 	} else {
@@ -150,6 +148,7 @@
  *  cmd_create
  *
  *  The 'create <options>' first level command
+ *  Returns one of the CMD_* result constants.
  */
 int cmd_create(int argc, const char **argv)
 {
------------------------------

   Turning to lttng-tools lib/lttng-ctl/lttng-ctl.c, this second patch documents the return values.

   The next patch is below this one.
------------------------------
From 0f8c95f984c933131c924681aa85d8cf4aa0d3a8 Fri, 27 Jan 2012 15:18:49 -0500
From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
Date: Fri, 27 Jan 2012 15:18:36 -0500
Subject: [PATCH] lttng-tools lib/lttng-ctl/lttng-ctl.c : Documenting return values

diff --git a/src/lib/lttng-ctl/lttng-ctl.c b/src/lib/lttng-ctl/lttng-ctl.c
index ebb76ec..4d11acc 100644
--- a/src/lib/lttng-ctl/lttng-ctl.c
+++ b/src/lib/lttng-ctl/lttng-ctl.c
@@ -83,8 +83,8 @@
 /*
  * Send lttcomm_session_msg to the session daemon.
  *
- * On success, return 0
- * On error, return error code
+ * On success, returns the number of bytes sent (>=0)
+ * On error, returns -1
  */
 static int send_session_msg(struct lttcomm_session_msg *lsm)
 {
@@ -105,8 +105,8 @@
 /*
  * Receive data from the sessiond socket.
  *
- * On success, return 0
- * On error, return recv() error code
+ * On success, returns the number of bytes received (>=0)
+ * On error, returns -1 (recvmsg() error) or -ENOTCONN
  */
 static int recv_data_sessiond(void *buf, size_t len)
 {
@@ -124,7 +124,7 @@
 }
 
 /*
- *  Check if the specified group name exist.
+ *  Check if we are in the specified group.
  *
  *  If yes return 1, else return -1.
  */
@@ -209,8 +209,10 @@
 }
 
 /*
- * Set sessiond socket path by putting it in the global sessiond_sock_path
- * variable.
+ * Set sessiond socket path by putting it in the global
+ * sessiond_sock_path variable.
+ * Returns 0 on success,
+ * -ENOMEM on failure (the sessiond socket path is somehow too long)
  */
 static int set_session_daemon_path(void)
 {
@@ -284,7 +286,8 @@
 }
 
 /*
- *  Clean disconnect the session daemon.
+ *  Clean disconnect from the session daemon.
+ *  On success, return 0. On error, return -1.
  */
 static int disconnect_sessiond(void)
 {
@@ -373,6 +376,7 @@
 
 /*
  * Create lttng handle and return pointer.
+ * The returned pointer will be NULL in case of malloc() error.
  */
 struct lttng_handle *lttng_create_handle(const char *session_name,
 		struct lttng_domain *domain)
@@ -408,6 +412,7 @@
 
 /*
  * Register an outside consumer.
+ * Returns size of returned session payload data or a negative error code.
  */
 int lttng_register_consumer(struct lttng_handle *handle,
 		const char *socket_path)
@@ -425,7 +430,8 @@
 }
 
 /*
- *  Start tracing for all trace of the session.
+ *  Start tracing for all traces of the session.
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_start_tracing(const char *session_name)
 {
@@ -443,7 +449,8 @@
 }
 
 /*
- *  Stop tracing for all trace of the session.
+ *  Stop tracing for all traces of the session.
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_stop_tracing(const char *session_name)
 {
@@ -495,7 +502,10 @@
 }
 
 /*
- * Enable event
+ *  Enable event(s) for a channel.
+ *  If no event name is specified, all events are enabled.
+ *  If no channel name is specified, the default 'channel0' is used.
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_enable_event(struct lttng_handle *handle,
 		struct lttng_event *ev, const char *channel_name)
@@ -531,7 +541,10 @@
 }
 
 /*
- * Disable event of a channel and domain.
+ *  Disable event(s) of a channel and domain.
+ *  If no event name is specified, all events are disabled.
+ *  If no channel name is specified, the default 'channel0' is used.
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_disable_event(struct lttng_handle *handle, const char *name,
 		const char *channel_name)
@@ -566,7 +579,8 @@
 }
 
 /*
- * Enable channel per domain
+ *  Enable channel per domain
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_enable_channel(struct lttng_handle *handle,
 		struct lttng_channel *chan)
@@ -593,7 +607,8 @@
 }
 
 /*
- * All tracing will be stopped for registered events of the channel.
+ *  All tracing will be stopped for registered events of the channel.
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_disable_channel(struct lttng_handle *handle, const char *name)
 {
@@ -618,10 +633,10 @@
 }
 
 /*
- * List all available tracepoints of domain.
- *
- * Return the size (bytes) of the list and set the events array.
- * On error, return negative value.
+ *  Lists all available tracepoints of domain.
+ *  Sets the contents of the events array.
+ *  Returns the number of lttng_event entries in events;
+ *  on error, returns a negative value.
  */
 int lttng_list_tracepoints(struct lttng_handle *handle,
 		struct lttng_event **events)
@@ -645,10 +660,12 @@
 }
 
 /*
- *  Return a human readable string of code
+ *  Returns a human readable string describing
+ *  the error code (a negative value).
  */
 const char *lttng_strerror(int code)
 {
+	/* lttcomm error codes range from -LTTCOMM_OK down to -LTTCOMM_NR */
 	if (code > -LTTCOMM_OK) {
 		return "Ended with errors";
 	}
@@ -657,7 +674,8 @@
 }
 
 /*
- *  Create a brand new session using name.
+ *  Create a brand new session using name and path.
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_create_session(const char *name, const char *path)
 {
@@ -672,6 +690,7 @@
 
 /*
  *  Destroy session using name.
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_destroy_session(const char *session_name)
 {
@@ -690,9 +709,9 @@
 
 /*
  *  Ask the session daemon for all available sessions.
- *
- *  Return number of session.
- *  On error, return negative value.
+ *  Sets the contents of the sessions array.
+ *  Returns the number of lttng_session entries in sessions;
+ *  on error, returns a negative value.
  */
 int lttng_list_sessions(struct lttng_session **sessions)
 {
@@ -709,7 +728,10 @@
 }
 
 /*
- * List domain of a session.
+ *  Ask the session daemon for all available domains of a session.
+ *  Sets the contents of the domains array.
+ *  Returns the number of lttng_domain entries in domains;
+ *  on error, returns a negative value.
  */
 int lttng_list_domains(const char *session_name,
 		struct lttng_domain **domains)
@@ -734,7 +756,10 @@
 }
 
 /*
- * List channels of a session
+ *  Ask the session daemon for all available channels of a session.
+ *  Sets the contents of the channels array.
+ *  Returns the number of lttng_channel entries in channels;
+ *  on error, returns a negative value.
  */
 int lttng_list_channels(struct lttng_handle *handle,
 		struct lttng_channel **channels)
@@ -761,7 +786,10 @@
 }
 
 /*
- * List events of a session channel.
+ *  Ask the session daemon for all available events of a session channel.
+ *  Sets the contents of the events array.
+ *  Returns the number of lttng_event entries in events;
+ *  on error, returns a negative value.
  */
 int lttng_list_events(struct lttng_handle *handle,
 		const char *channel_name, struct lttng_event **events)
@@ -791,8 +819,9 @@
 }
 
 /*
- * Set tracing group variable with name. This function allocate memory pointed
- * by tracing_group.
+ * Sets the tracing_group variable with name.
+ * This function allocates memory pointed to by tracing_group.
+ * On success, returns 0, on error, returns -1 (null name) or -ENOMEM.
  */
 int lttng_set_tracing_group(const char *name)
 {
@@ -831,6 +860,7 @@
 
 /*
  * Set default channel attributes.
+ * If either or both of the arguments are null, nothing happens.
  */
 void lttng_channel_set_default_attr(struct lttng_domain *domain,
 		struct lttng_channel_attr *attr)
@@ -875,7 +905,7 @@
  * Check if session daemon is alive.
  *
  * Return 1 if alive or 0 if not.
- * On error return -1
+ * On error returns a negative value.
  */
 int lttng_session_daemon_alive(void)
 {
------------------------------

   The third patch turns to lttng-ctl.c's code.

* In check_tracing_group(), we had grp_id = getgroups(...); if (grp_id < -1) { ... but the documentation for getgroups() clearly indicates its return value can never be < -1

* In set_session_daemon_path(), there were several snippets of code that were repeated inelegantly.  This is more than just a matter of readability and elegance: if a snippet is repeated (with possibly minor changes), any maintenance on one occurrence runs the risk of not being correctly repeated on the others.  I rewrote the set of ifs in order to achieve the same behaviour without the aforementioned repetitions.

* In the same function, the test of snprintf()'s return value is valid only for GNU C before version 2.1; since 2.1, snprintf() no longer fails if the target buffer is too small: instead it writes as much as it can (preserving the string's closing null) and then returns the amount of space it would have liked to have (excluding the string's closing null).  I rewrote the if so it works in either case.

* In connect_sessiond(), a self-explanatory correction.

* In lttng_enable_event() and lttng_disable_event(), the two variants of copy_string() enclosed by if (channel_name) were replaced by a single copy_string using the ? operator.

------------------------------
From 082c501bc84f60ae8e422027cc2856cb3fd90233 Fri, 27 Jan 2012 15:39:14 -0500
From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
Date: Fri, 27 Jan 2012 15:39:04 -0500
Subject: [PATCH] lttng-tools lib/lttng-ctl/lttng-ctl.c : Various code fixes

diff --git a/lttng2-lttng-tools/src/lib/lttng-ctl/lttng-ctl.c b/lttng2-lttng-tools/src/lib/lttng-ctl/lttng-ctl.c
index 4d11acc..55a7003 100644
--- a/lttng2-lttng-tools/src/lib/lttng-ctl/lttng-ctl.c
+++ b/lttng2-lttng-tools/src/lib/lttng-ctl/lttng-ctl.c
@@ -153,13 +153,15 @@
 	}
 
 	/* Alloc group list of the right size */
+    /* Note that malloc(0) will return a valid pointer */
 	grp_list = malloc(grp_list_size * sizeof(gid_t));
 	if (!grp_list) {
-		ret = -1;
+		perror("malloc");
 		goto end;
 	}
+
 	grp_id = getgroups(grp_list_size, grp_list);
-	if (grp_id < -1) {
+	if (grp_id < 0) {
 		perror("getgroups");
 		goto free_list;
 	}
@@ -227,35 +229,31 @@
 		in_tgroup = check_tracing_group(tracing_group);
 	}
 
-	if (uid == 0) {
-		/* Root */
+	if ((uid == 0) || in_tgroup) {
 		copy_string(sessiond_sock_path,
 				DEFAULT_GLOBAL_CLIENT_UNIX_SOCK,
 				sizeof(sessiond_sock_path));
-	} else if (in_tgroup) {
-		/* Tracing group */
-		copy_string(sessiond_sock_path,
-				DEFAULT_GLOBAL_CLIENT_UNIX_SOCK,
-				sizeof(sessiond_sock_path));
-
-		ret = try_connect_sessiond(sessiond_sock_path);
-		if (ret < 0) {
-			/* Global session daemon not available */
-			if (snprintf(sessiond_sock_path, sizeof(sessiond_sock_path),
-						DEFAULT_HOME_CLIENT_UNIX_SOCK,
-						getenv("HOME")) < 0) {
-				return -ENOMEM;
-			}
+	}
+	if (uid != 0) {
+		if (in_tgroup) {
+			/* Tracing group */
+			ret = try_connect_sessiond(sessiond_sock_path);
+			if (ret >= 0) goto end;
+			/* Global session daemon not available... */
 		}
-	} else {
-		/* Not in tracing group and not root, default */
-		if (snprintf(sessiond_sock_path, PATH_MAX,
-					DEFAULT_HOME_CLIENT_UNIX_SOCK,
-					getenv("HOME")) < 0) {
+		/* ...or not in tracing group (and not root), default */
+		/*
+		 * With GNU C <  2.1, snprintf returns -1 if the target buffer is too small;
+		 * With GNU C >= 2.1, snprintf returns the required size (excluding closing null)
+		 */
+		ret = snprintf(sessiond_sock_path, sizeof(sessiond_sock_path),
+				DEFAULT_HOME_CLIENT_UNIX_SOCK,
+				getenv("HOME"));
+		if ((ret < 0) || (ret >= sizeof(sessiond_sock_path))) {
 			return -ENOMEM;
 		}
 	}
-
+end:
 	return 0;
 }
 
@@ -276,7 +274,7 @@
 	/* Connect to the sesssion daemon */
 	ret = lttcomm_connect_unix_sock(sessiond_sock_path);
 	if (ret < 0) {
-		return ret;
+		return -1; /* set_session_daemon_path() returns -ENOMEM */
 	}
 
 	sessiond_socket = ret;
@@ -517,13 +515,9 @@
 	}
 
 	/* If no channel name, we put the default name */
-	if (channel_name == NULL) {
-		copy_string(lsm.u.enable.channel_name, DEFAULT_CHANNEL_NAME,
-				sizeof(lsm.u.enable.channel_name));
-	} else {
-		copy_string(lsm.u.enable.channel_name, channel_name,
-				sizeof(lsm.u.enable.channel_name));
-	}
+	copy_string(lsm.u.enable.channel_name,
+			channel_name ? channel_name : DEFAULT_CHANNEL_NAME,
+			sizeof(lsm.u.enable.channel_name));
 
 	copy_lttng_domain(&lsm.domain, &handle->domain);
 
@@ -555,13 +549,9 @@
 		return -1;
 	}
 
-	if (channel_name) {
-		copy_string(lsm.u.disable.channel_name, channel_name,
-				sizeof(lsm.u.disable.channel_name));
-	} else {
-		copy_string(lsm.u.disable.channel_name, DEFAULT_CHANNEL_NAME,
-				sizeof(lsm.u.disable.channel_name));
-	}
+	copy_string(lsm.u.disable.channel_name,
+			channel_name ? channel_name : DEFAULT_CHANNEL_NAME,
+			sizeof(lsm.u.disable.channel_name));
 
 	copy_lttng_domain(&lsm.domain, &handle->domain);
------------------------------

   One remaining issue is ask_sessiond(), which still does not compare the number of bytes received with the number requested.  What should it do if they don't match?  Throw everything away and report an error?

   From here, the 0b675c0c4cba0c9bbbd9770005d7f260ddbf2a45 patch can be applied.

   I've been learning quite a bit about git through this exercise.  :-)

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] [RESUBMISSION] lttng-tools lib/lttng-ctl/lttng-ctl.c : Document return values and more

[David Goulet]

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

Hmmmm,

I'm having problem applying those patches. It fails at the third one and I think
it's because you change lttng-ctl.c in the second patch and the third one is not
based on those changes.

Can you please send me three separate emails for those patches (one per mail)
and had your "Signed-off-by" also (just add "-s" to git commit, it will do it
automatically for you). It helps me a lot for review and merge.

Thanks a lot! Really appreciate the documentation and cleanup job you are doing.

Cheers
David

On 12-01-27 03:49 PM, Thibault, Daniel wrote:
>> However, there is a lot of comments added along with important modifications to two functions.
>>
>> Can you please make three different patches for that (ideally with git format-patch :)). Especially for the
>> modifications in liblttng-ctl which is quite a core function.
>>
>> It's very important to separate those kind of changes in order for us to easily "git bisect/blame" and pin
>> point commit/committer if anything goes wrong with the code.
> 
>    All right, backing up to my commit 9de81398942fe0adeb5288483ecb70e2573e69f0 and patching in three steps from there, here's a first patch that affects just lttng-tools bin/lttng/commands/create.c :
> 
> * Improve usage() output
> * Document and enforce return values
> * Simplify create_session() name handling
> 
>    The next patch is below this one.
> ------------------------------
> From b58d8e97280d3cd87ae187bb79c4800f39a31f64 Fri, 27 Jan 2012 15:09:58 -0500
> From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
> Date: Fri, 27 Jan 2012 15:09:35 -0500
> Subject: [PATCH] lttng-tools bin/lttng/commands/create.c : Improve usage(), document and enforce return values, simplify create_session() name handling
> 
> diff --git a/src/bin/lttng/commands/create.c b/src/bin/lttng/commands/create.c
> index 34754c7..94ff634 100644
> --- a/src/bin/lttng/commands/create.c
> +++ b/src/bin/lttng/commands/create.c
> @@ -52,8 +52,9 @@
>  {
>  	fprintf(ofp, "usage: lttng create [options] [NAME]\n");
>  	fprintf(ofp, "\n");
> +	fprintf(ofp, "  The default NAME is 'auto-yyyymmdd-hhmmss'\n");
>  	fprintf(ofp, "  -h, --help           Show this help\n");
> -	fprintf(ofp, "      --list-options       Simple listing of options\n");
> +	fprintf(ofp, "      --list-options   Simple listing of options\n");
>  	fprintf(ofp, "  -o, --output PATH    Specify output path for traces\n");
>  	fprintf(ofp, "\n");
>  }
> @@ -61,12 +62,13 @@
>  /*
>   *  create_session
>   *
> - *  Create a tracing session. If no name specified, a default name will be
> - *  generated.
> + *  Create a tracing session.
> + *  If no name is specified, a default name is generated.
> + *  Returns one of the CMD_* result constants.
>   */
>  static int create_session()
>  {
> -	int ret, have_name = 0;
> +	int ret;
>  	char datetime[16];
>  	char *session_name, *traces_path = NULL, *alloc_path = NULL;
>  	time_t rawtime;
> @@ -79,37 +81,33 @@
>  
>  	/* Auto session name creation */
>  	if (opt_session_name == NULL) {
> -		ret = asprintf(&session_name, "auto-%s", datetime);
> +		ret = asprintf(&session_name, "auto");
>  		if (ret < 0) {
>  			perror("asprintf session name");
> +			ret = CMD_ERROR;
>  			goto error;
>  		}
>  		DBG("Auto session name set to %s", session_name);
>  	} else {
>  		session_name = opt_session_name;
> -		have_name = 1;
>  	}
>  
>  	/* Auto output path */
>  	if (opt_output_path == NULL) {
>  		alloc_path = strdup(config_get_default_path());
>  		if (alloc_path == NULL) {
> -			ERR("Home path not found.\n \
> -				 Please specify an output path using -o, --output PATH");
> +			ERR("Home path not found.\n%s"
> +				"Please specify an output path using -o, --output PATH\n");
>  			ret = CMD_FATAL;
>  			goto error;
>  		}
>  
> -		if (have_name) {
> -			ret = asprintf(&traces_path, "%s/" DEFAULT_TRACE_DIR_NAME
> +		ret = asprintf(&traces_path, "%s/" DEFAULT_TRACE_DIR_NAME
>  					"/%s-%s", alloc_path, session_name, datetime);
> -		} else {
> -			ret = asprintf(&traces_path, "%s/" DEFAULT_TRACE_DIR_NAME
> -					"/%s", alloc_path, session_name);
> -		}
>  
>  		if (ret < 0) {
>  			perror("asprintf trace dir name");
> +			ret = CMD_ERROR;
>  			goto error;
>  		}
>  	} else {
> @@ -150,6 +148,7 @@
>   *  cmd_create
>   *
>   *  The 'create <options>' first level command
> + *  Returns one of the CMD_* result constants.
>   */
>  int cmd_create(int argc, const char **argv)
>  {
> ------------------------------
> 
>    Turning to lttng-tools lib/lttng-ctl/lttng-ctl.c, this second patch documents the return values.
> 
>    The next patch is below this one.
> ------------------------------
> From 0f8c95f984c933131c924681aa85d8cf4aa0d3a8 Fri, 27 Jan 2012 15:18:49 -0500
> From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
> Date: Fri, 27 Jan 2012 15:18:36 -0500
> Subject: [PATCH] lttng-tools lib/lttng-ctl/lttng-ctl.c : Documenting return values
> 
> diff --git a/src/lib/lttng-ctl/lttng-ctl.c b/src/lib/lttng-ctl/lttng-ctl.c
> index ebb76ec..4d11acc 100644
> --- a/src/lib/lttng-ctl/lttng-ctl.c
> +++ b/src/lib/lttng-ctl/lttng-ctl.c
> @@ -83,8 +83,8 @@
>  /*
>   * Send lttcomm_session_msg to the session daemon.
>   *
> - * On success, return 0
> - * On error, return error code
> + * On success, returns the number of bytes sent (>=0)
> + * On error, returns -1
>   */
>  static int send_session_msg(struct lttcomm_session_msg *lsm)
>  {
> @@ -105,8 +105,8 @@
>  /*
>   * Receive data from the sessiond socket.
>   *
> - * On success, return 0
> - * On error, return recv() error code
> + * On success, returns the number of bytes received (>=0)
> + * On error, returns -1 (recvmsg() error) or -ENOTCONN
>   */
>  static int recv_data_sessiond(void *buf, size_t len)
>  {
> @@ -124,7 +124,7 @@
>  }
>  
>  /*
> - *  Check if the specified group name exist.
> + *  Check if we are in the specified group.
>   *
>   *  If yes return 1, else return -1.
>   */
> @@ -209,8 +209,10 @@
>  }
>  
>  /*
> - * Set sessiond socket path by putting it in the global sessiond_sock_path
> - * variable.
> + * Set sessiond socket path by putting it in the global
> + * sessiond_sock_path variable.
> + * Returns 0 on success,
> + * -ENOMEM on failure (the sessiond socket path is somehow too long)
>   */
>  static int set_session_daemon_path(void)
>  {
> @@ -284,7 +286,8 @@
>  }
>  
>  /*
> - *  Clean disconnect the session daemon.
> + *  Clean disconnect from the session daemon.
> + *  On success, return 0. On error, return -1.
>   */
>  static int disconnect_sessiond(void)
>  {
> @@ -373,6 +376,7 @@
>  
>  /*
>   * Create lttng handle and return pointer.
> + * The returned pointer will be NULL in case of malloc() error.
>   */
>  struct lttng_handle *lttng_create_handle(const char *session_name,
>  		struct lttng_domain *domain)
> @@ -408,6 +412,7 @@
>  
>  /*
>   * Register an outside consumer.
> + * Returns size of returned session payload data or a negative error code.
>   */
>  int lttng_register_consumer(struct lttng_handle *handle,
>  		const char *socket_path)
> @@ -425,7 +430,8 @@
>  }
>  
>  /*
> - *  Start tracing for all trace of the session.
> + *  Start tracing for all traces of the session.
> + *  Returns size of returned session payload data or a negative error code.
>   */
>  int lttng_start_tracing(const char *session_name)
>  {
> @@ -443,7 +449,8 @@
>  }
>  
>  /*
> - *  Stop tracing for all trace of the session.
> + *  Stop tracing for all traces of the session.
> + *  Returns size of returned session payload data or a negative error code.
>   */
>  int lttng_stop_tracing(const char *session_name)
>  {
> @@ -495,7 +502,10 @@
>  }
>  
>  /*
> - * Enable event
> + *  Enable event(s) for a channel.
> + *  If no event name is specified, all events are enabled.
> + *  If no channel name is specified, the default 'channel0' is used.
> + *  Returns size of returned session payload data or a negative error code.
>   */
>  int lttng_enable_event(struct lttng_handle *handle,
>  		struct lttng_event *ev, const char *channel_name)
> @@ -531,7 +541,10 @@
>  }
>  
>  /*
> - * Disable event of a channel and domain.
> + *  Disable event(s) of a channel and domain.
> + *  If no event name is specified, all events are disabled.
> + *  If no channel name is specified, the default 'channel0' is used.
> + *  Returns size of returned session payload data or a negative error code.
>   */
>  int lttng_disable_event(struct lttng_handle *handle, const char *name,
>  		const char *channel_name)
> @@ -566,7 +579,8 @@
>  }
>  
>  /*
> - * Enable channel per domain
> + *  Enable channel per domain
> + *  Returns size of returned session payload data or a negative error code.
>   */
>  int lttng_enable_channel(struct lttng_handle *handle,
>  		struct lttng_channel *chan)
> @@ -593,7 +607,8 @@
>  }
>  
>  /*
> - * All tracing will be stopped for registered events of the channel.
> + *  All tracing will be stopped for registered events of the channel.
> + *  Returns size of returned session payload data or a negative error code.
>   */
>  int lttng_disable_channel(struct lttng_handle *handle, const char *name)
>  {
> @@ -618,10 +633,10 @@
>  }
>  
>  /*
> - * List all available tracepoints of domain.
> - *
> - * Return the size (bytes) of the list and set the events array.
> - * On error, return negative value.
> + *  Lists all available tracepoints of domain.
> + *  Sets the contents of the events array.
> + *  Returns the number of lttng_event entries in events;
> + *  on error, returns a negative value.
>   */
>  int lttng_list_tracepoints(struct lttng_handle *handle,
>  		struct lttng_event **events)
> @@ -645,10 +660,12 @@
>  }
>  
>  /*
> - *  Return a human readable string of code
> + *  Returns a human readable string describing
> + *  the error code (a negative value).
>   */
>  const char *lttng_strerror(int code)
>  {
> +	/* lttcomm error codes range from -LTTCOMM_OK down to -LTTCOMM_NR */
>  	if (code > -LTTCOMM_OK) {
>  		return "Ended with errors";
>  	}
> @@ -657,7 +674,8 @@
>  }
>  
>  /*
> - *  Create a brand new session using name.
> + *  Create a brand new session using name and path.
> + *  Returns size of returned session payload data or a negative error code.
>   */
>  int lttng_create_session(const char *name, const char *path)
>  {
> @@ -672,6 +690,7 @@
>  
>  /*
>   *  Destroy session using name.
> + *  Returns size of returned session payload data or a negative error code.
>   */
>  int lttng_destroy_session(const char *session_name)
>  {
> @@ -690,9 +709,9 @@
>  
>  /*
>   *  Ask the session daemon for all available sessions.
> - *
> - *  Return number of session.
> - *  On error, return negative value.
> + *  Sets the contents of the sessions array.
> + *  Returns the number of lttng_session entries in sessions;
> + *  on error, returns a negative value.
>   */
>  int lttng_list_sessions(struct lttng_session **sessions)
>  {
> @@ -709,7 +728,10 @@
>  }
>  
>  /*
> - * List domain of a session.
> + *  Ask the session daemon for all available domains of a session.
> + *  Sets the contents of the domains array.
> + *  Returns the number of lttng_domain entries in domains;
> + *  on error, returns a negative value.
>   */
>  int lttng_list_domains(const char *session_name,
>  		struct lttng_domain **domains)
> @@ -734,7 +756,10 @@
>  }
>  
>  /*
> - * List channels of a session
> + *  Ask the session daemon for all available channels of a session.
> + *  Sets the contents of the channels array.
> + *  Returns the number of lttng_channel entries in channels;
> + *  on error, returns a negative value.
>   */
>  int lttng_list_channels(struct lttng_handle *handle,
>  		struct lttng_channel **channels)
> @@ -761,7 +786,10 @@
>  }
>  
>  /*
> - * List events of a session channel.
> + *  Ask the session daemon for all available events of a session channel.
> + *  Sets the contents of the events array.
> + *  Returns the number of lttng_event entries in events;
> + *  on error, returns a negative value.
>   */
>  int lttng_list_events(struct lttng_handle *handle,
>  		const char *channel_name, struct lttng_event **events)
> @@ -791,8 +819,9 @@
>  }
>  
>  /*
> - * Set tracing group variable with name. This function allocate memory pointed
> - * by tracing_group.
> + * Sets the tracing_group variable with name.
> + * This function allocates memory pointed to by tracing_group.
> + * On success, returns 0, on error, returns -1 (null name) or -ENOMEM.
>   */
>  int lttng_set_tracing_group(const char *name)
>  {
> @@ -831,6 +860,7 @@
>  
>  /*
>   * Set default channel attributes.
> + * If either or both of the arguments are null, nothing happens.
>   */
>  void lttng_channel_set_default_attr(struct lttng_domain *domain,
>  		struct lttng_channel_attr *attr)
> @@ -875,7 +905,7 @@
>   * Check if session daemon is alive.
>   *
>   * Return 1 if alive or 0 if not.
> - * On error return -1
> + * On error returns a negative value.
>   */
>  int lttng_session_daemon_alive(void)
>  {
> ------------------------------
> 
>    The third patch turns to lttng-ctl.c's code.
> 
> * In check_tracing_group(), we had grp_id = getgroups(...); if (grp_id < -1) { ... but the documentation for getgroups() clearly indicates its return value can never be < -1
> 
> * In set_session_daemon_path(), there were several snippets of code that were repeated inelegantly.  This is more than just a matter of readability and elegance: if a snippet is repeated (with possibly minor changes), any maintenance on one occurrence runs the risk of not being correctly repeated on the others.  I rewrote the set of ifs in order to achieve the same behaviour without the aforementioned repetitions.
> 
> * In the same function, the test of snprintf()'s return value is valid only for GNU C before version 2.1; since 2.1, snprintf() no longer fails if the target buffer is too small: instead it writes as much as it can (preserving the string's closing null) and then returns the amount of space it would have liked to have (excluding the string's closing null).  I rewrote the if so it works in either case.
> 
> * In connect_sessiond(), a self-explanatory correction.
> 
> * In lttng_enable_event() and lttng_disable_event(), the two variants of copy_string() enclosed by if (channel_name) were replaced by a single copy_string using the ? operator.
> 
> ------------------------------
> From 082c501bc84f60ae8e422027cc2856cb3fd90233 Fri, 27 Jan 2012 15:39:14 -0500
> From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
> Date: Fri, 27 Jan 2012 15:39:04 -0500
> Subject: [PATCH] lttng-tools lib/lttng-ctl/lttng-ctl.c : Various code fixes
> 
> diff --git a/lttng2-lttng-tools/src/lib/lttng-ctl/lttng-ctl.c b/lttng2-lttng-tools/src/lib/lttng-ctl/lttng-ctl.c
> index 4d11acc..55a7003 100644
> --- a/lttng2-lttng-tools/src/lib/lttng-ctl/lttng-ctl.c
> +++ b/lttng2-lttng-tools/src/lib/lttng-ctl/lttng-ctl.c
> @@ -153,13 +153,15 @@
>  	}
>  
>  	/* Alloc group list of the right size */
> +    /* Note that malloc(0) will return a valid pointer */
>  	grp_list = malloc(grp_list_size * sizeof(gid_t));
>  	if (!grp_list) {
> -		ret = -1;
> +		perror("malloc");
>  		goto end;
>  	}
> +
>  	grp_id = getgroups(grp_list_size, grp_list);
> -	if (grp_id < -1) {
> +	if (grp_id < 0) {
>  		perror("getgroups");
>  		goto free_list;
>  	}
> @@ -227,35 +229,31 @@
>  		in_tgroup = check_tracing_group(tracing_group);
>  	}
>  
> -	if (uid == 0) {
> -		/* Root */
> +	if ((uid == 0) || in_tgroup) {
>  		copy_string(sessiond_sock_path,
>  				DEFAULT_GLOBAL_CLIENT_UNIX_SOCK,
>  				sizeof(sessiond_sock_path));
> -	} else if (in_tgroup) {
> -		/* Tracing group */
> -		copy_string(sessiond_sock_path,
> -				DEFAULT_GLOBAL_CLIENT_UNIX_SOCK,
> -				sizeof(sessiond_sock_path));
> -
> -		ret = try_connect_sessiond(sessiond_sock_path);
> -		if (ret < 0) {
> -			/* Global session daemon not available */
> -			if (snprintf(sessiond_sock_path, sizeof(sessiond_sock_path),
> -						DEFAULT_HOME_CLIENT_UNIX_SOCK,
> -						getenv("HOME")) < 0) {
> -				return -ENOMEM;
> -			}
> +	}
> +	if (uid != 0) {
> +		if (in_tgroup) {
> +			/* Tracing group */
> +			ret = try_connect_sessiond(sessiond_sock_path);
> +			if (ret >= 0) goto end;
> +			/* Global session daemon not available... */
>  		}
> -	} else {
> -		/* Not in tracing group and not root, default */
> -		if (snprintf(sessiond_sock_path, PATH_MAX,
> -					DEFAULT_HOME_CLIENT_UNIX_SOCK,
> -					getenv("HOME")) < 0) {
> +		/* ...or not in tracing group (and not root), default */
> +		/*
> +		 * With GNU C <  2.1, snprintf returns -1 if the target buffer is too small;
> +		 * With GNU C >= 2.1, snprintf returns the required size (excluding closing null)
> +		 */
> +		ret = snprintf(sessiond_sock_path, sizeof(sessiond_sock_path),
> +				DEFAULT_HOME_CLIENT_UNIX_SOCK,
> +				getenv("HOME"));
> +		if ((ret < 0) || (ret >= sizeof(sessiond_sock_path))) {
>  			return -ENOMEM;
>  		}
>  	}
> -
> +end:
>  	return 0;
>  }
>  
> @@ -276,7 +274,7 @@
>  	/* Connect to the sesssion daemon */
>  	ret = lttcomm_connect_unix_sock(sessiond_sock_path);
>  	if (ret < 0) {
> -		return ret;
> +		return -1; /* set_session_daemon_path() returns -ENOMEM */
>  	}
>  
>  	sessiond_socket = ret;
> @@ -517,13 +515,9 @@
>  	}
>  
>  	/* If no channel name, we put the default name */
> -	if (channel_name == NULL) {
> -		copy_string(lsm.u.enable.channel_name, DEFAULT_CHANNEL_NAME,
> -				sizeof(lsm.u.enable.channel_name));
> -	} else {
> -		copy_string(lsm.u.enable.channel_name, channel_name,
> -				sizeof(lsm.u.enable.channel_name));
> -	}
> +	copy_string(lsm.u.enable.channel_name,
> +			channel_name ? channel_name : DEFAULT_CHANNEL_NAME,
> +			sizeof(lsm.u.enable.channel_name));
>  
>  	copy_lttng_domain(&lsm.domain, &handle->domain);
>  
> @@ -555,13 +549,9 @@
>  		return -1;
>  	}
>  
> -	if (channel_name) {
> -		copy_string(lsm.u.disable.channel_name, channel_name,
> -				sizeof(lsm.u.disable.channel_name));
> -	} else {
> -		copy_string(lsm.u.disable.channel_name, DEFAULT_CHANNEL_NAME,
> -				sizeof(lsm.u.disable.channel_name));
> -	}
> +	copy_string(lsm.u.disable.channel_name,
> +			channel_name ? channel_name : DEFAULT_CHANNEL_NAME,
> +			sizeof(lsm.u.disable.channel_name));
>  
>  	copy_lttng_domain(&lsm.domain, &handle->domain);
> ------------------------------
> 
>    One remaining issue is ask_sessiond(), which still does not compare the number of bytes received with the number requested.  What should it do if they don't match?  Throw everything away and report an error?
> 
>    From here, the 0b675c0c4cba0c9bbbd9770005d7f260ddbf2a45 patch can be applied.
> 
>    I've been learning quite a bit about git through this exercise.  :-)
> 
> 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/>
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iQEcBAEBAgAGBQJPJt1gAAoJEELoaioR9I02Ny0IAMLxNQPEI1Aay2f4puXSywRT
LRCTf9tXYBJAgJEc5nIRn0F5bmkvMGgSYUKqeXCbx1n7u/ryXo61H3pgFtX5CsuX
Yjjs9aWKa5HIHfmKDpccOUaAi5hySXsOyLuG2VzhRL7JuYsOQLf8eFrSybuM4dFB
X6JG2o0Ru8pDwDRhwYRJHbKqwRqNDkuZcpyWZShNFSuCWC5ITw4WFfxYco/I51dB
gKaOubZEA07VZTe6bsgHlWhH3TaAIpntWXtZU2w1LkZCmQ2PyX5JP4Yd6eCmdCF2
h7j7gR1hhCdanmvZj7v8+A/E70ClsB07B/Rrz0UYvhdWJJzSPG6yv6p1TtrK0kc=
=gfYU
-----END PGP SIGNATURE-----

[lttng-dev] [PATCH] [RESUBMISSION] lttng-tools lib/lttng-ctl/lttng-ctl.c : Document return values and more

[Thibault, Daniel]

-----Message d'origine-----
De?: David Goulet [mailto:david.goulet at polymtl.ca] 
Envoy??: 30 janvier 2012 13:12

> I'm having problem applying those patches. It fails at the third one and I think
> it's because you change lttng-ctl.c in the second patch and the third one is not
> based on those changes.

   But it is!?

> Can you please send me three separate emails for those patches (one per mail)
> and had your "Signed-off-by" also (just add "-s" to git commit, it will do it
> automatically for you). It helps me a lot for review and merge.

   Er...I don't seem to have the option of adding "-s", 'cause I'm using EGit from Eclipse.  The only options I get when producing a patch are a) output to clipboard or file, b) export in git patch format or not, and c) the number of lines of context.  Never mind, found it in Window: Preferences: Team: Git: Commit dialog.

   Now, out here we have a separate problem getting at the git repositories using the git: protocol.  The port isn't open in out firewall.  A request has been submitted to get that opened.  Meantime, I must download a local copy of the git commits and import them as regular projects before "gitifying" them.  Bottom line: it works but there are extra hoops to jump through for now.  Once our firewall problem is solved, it'll be a lot easier for me to follow the code's evolution.

   So, in order to fix the problem you're having with the "third patch" ([PATCH] lttng-tools lib/lttng-ctl/lttng-ctl.c : Various code fixes), I'll download the current version (I see I'm 15 commits behind) and regenerate patches from there.

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] [RESUBMISSION] lttng-tools lib/lttng-ctl/lttng-ctl.c : Document return values and more

[Alexandre Montplaisir]

On 12-01-30 01:52 PM, Thibault, Daniel wrote:
> -----Message d'origine-----
> De : David Goulet [mailto:david.goulet at polymtl.ca] 
> Envoy? : 30 janvier 2012 13:12
>
>
>    Now, out here we have a separate problem getting at the git repositories using the git: protocol.  The port isn't open in out firewall.  A request has been submitted to get that opened.  Meantime, I must download a local copy of the git commits and import them as regular projects before "gitifying" them.  Bottom line: it works but there are extra hoops to jump through for now.  Once our firewall problem is solved, it'll be a lot easier for me to follow the code's evolution.

Hi Daniel,

You can pull over HTTP, for example:

git clone http://git.lttng.org//lttng-tools.git

This usually gets through corporate firewalls.


Cheers,

-- 
Alexandre Montplaisir
DORSAL lab,
?cole Polytechnique de Montr?al

[lttng-dev] git: vs http:

[Thibault, Daniel]

-----Message d'origine-----
Envoy? : 30 janvier 2012 15:27

> You can pull over HTTP, for example:
>
> git clone http://git.lttng.org//lttng-tools.git
>
> This usually gets through corporate firewalls.

   One problem solved, I guess.  My problem was that I only tried the git: path (git://git.lttng.org/projects/lttng-tools.git) changed to http: as http://git.lttng.org/projects/lttng-tools.git ... and the skipped directory threw me.  Still, it'll be worthwhile to get the git port opened through our firewall.

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 Catching up on miscellaneous string and comment fixes (from 5c73c59)

[Thibault, Daniel]

   As promised, here is the first of a new set of patches, starting from the 2012-01-30 18:27 commit (2.0-pre18+-5c73c59).

   This first patch fixes miscellaneous little things wrong with strings and comments.

------------------------------
From cb6916eb52786f8caf1612c576500ca6bb6f4e76 Mon, 30 Jan 2012 15:20:12 -0500
From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
Date: Mon, 30 Jan 2012 15:19:50 -0500
Subject: [PATCH] lttng-tools Catching up on miscellaneous string and comment fixes (from 5c73c59)

Signed-off-by: Daniel U. Thibault <daniel.thibault at drdc-rddc.gc.ca>

diff --git a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng-consumerd/lttng-consumerd.c b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng-consumerd/lttng-consumerd.c
index 5d1ad06..7e3bf3c 100644
--- a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng-consumerd/lttng-consumerd.c
+++ b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng-consumerd/lttng-consumerd.c
@@ -49,7 +49,7 @@
 
 #include "lttng-consumerd.h"
 
-/* TODO : support UST (all direct kern-ctl accesses). */
+/* TODO : support UST (all direct kernel-ctl accesses). */
 
 /* the two threads (receive fd and poll) */
 static pthread_t threads[2];
@@ -310,7 +310,7 @@
 	ret = lttcomm_connect_unix_sock(error_sock_path);
 	/* not a fatal error, but all communication with lttng-sessiond will fail */
 	if (ret < 0) {
-		WARN("Cannot connect to error socket (is lttng-sessiond started ?)");
+		WARN("Cannot connect to error socket (is lttng-sessiond started?)");
 	}
 	lttng_consumer_set_error_sock(ctx, ret);
 
diff --git a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng-sessiond/main.c b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng-sessiond/main.c
index 9b2d1fe..8304de8 100644
--- a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng-sessiond/main.c
+++ b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng-sessiond/main.c
@@ -314,7 +314,7 @@
 static void teardown_kernel_session(struct ltt_session *session)
 {
 	if (!session->kernel_session) {
-		DBG3("No kernel session when tearingdown session");
+		DBG3("No kernel session when tearing down session");
 		return;
 	}
 
@@ -340,7 +340,7 @@
 	int ret;
 
 	if (!session->ust_session) {
-		DBG3("No UST session when tearingdown session");
+		DBG3("No UST session when tearing down session");
 		return;
 	}
 
@@ -397,7 +397,7 @@
 	}
 	free(cmd);
 
-	DBG("Cleaning up all session");
+	DBG("Cleaning up all sessions");
 
 	/* Destroy session list mutex */
 	if (session_list_ptr != NULL) {
diff --git a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/lttng.c b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/lttng.c
index 31b4714..9c34143 100644
--- a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/lttng.c
+++ b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/lttng.c
@@ -85,14 +85,14 @@
 	fprintf(ofp, "usage: lttng [OPTIONS] <COMMAND>\n");
 	fprintf(ofp, "\n");
 	fprintf(ofp, "Options:\n");
-	fprintf(ofp, "  -h, --help             Show this help\n");
-	fprintf(ofp, "      --list-options     Simple listing of lttng options\n");
-	fprintf(ofp, "      --list-commands    Simple listing of lttng commands\n");
-	fprintf(ofp, "  -v, --verbose          Increase verbosity\n");
-	fprintf(ofp, "  -q, --quiet            Quiet mode\n");
-	fprintf(ofp, "  -g, --group NAME       Unix tracing group name. (default: tracing)\n");
-	fprintf(ofp, "  -n, --no-sessiond      Don't spawn a session daemon\n");
-	fprintf(ofp, "      --sessiond-path    Session daemon full path\n");
+	fprintf(ofp, "  -h, --help               Show this help\n");
+	fprintf(ofp, "      --list-options       Simple listing of lttng options\n");
+	fprintf(ofp, "      --list-commands      Simple listing of lttng commands\n");
+	fprintf(ofp, "  -v, --verbose            Increase verbosity\n");
+	fprintf(ofp, "  -q, --quiet              Quiet mode\n");
+	fprintf(ofp, "  -g, --group NAME         Unix tracing group name. (default: tracing)\n");
+	fprintf(ofp, "  -n, --no-sessiond        Don't spawn a session daemon\n");
+	fprintf(ofp, "      --sessiond-path PATH Session daemon full path\n");
 	fprintf(ofp, "\n");
 	fprintf(ofp, "Commands:\n");
 	fprintf(ofp, "    add-context     Add context to event and/or channel\n");
@@ -108,6 +108,8 @@
 	fprintf(ofp, "    start           Start tracing\n");
 	fprintf(ofp, "    stop            Stop tracing\n");
 	fprintf(ofp, "    version         Show version information\n");
+	fprintf(ofp, "\n");
+	fprintf(ofp, "Each command also has its own -h, --help option.\n");
 	fprintf(ofp, "\n");
 	fprintf(ofp, "Please see the lttng(1) man page for full documentation.\n");
 	fprintf(ofp, "See http://lttng.org for updates, bug reports and news.\n");
@@ -176,11 +178,11 @@
 
 	switch (sig) {
 		case SIGTERM:
-			DBG("SIGTERM caugth");
+			DBG("SIGTERM caught");
 			clean_exit(EXIT_FAILURE);
 			break;
 		case SIGCHLD:
-			DBG("SIGCHLD caugth");
+			DBG("SIGCHLD caught");
 			waitpid(sessiond_pid, &status, 0);
 			recv_child_signal = 1;
 			/* Indicate that the session daemon died */
@@ -190,10 +192,10 @@
 		case SIGUSR1:
 			/* Notify is done */
 			recv_child_signal = 1;
-			DBG("SIGUSR1 caugth");
+			DBG("SIGUSR1 caught");
 			break;
 		default:
-			DBG("Unknown signal %d caugth", sig);
+			DBG("Unknown signal %d caught", sig);
 			break;
 	}
 
@@ -514,7 +516,7 @@
 
 	progname = argv[0] ? argv[0] : "lttng";
 
-	/* For Mathieu Desnoyers aka Dr Tracing */
+	/* For Mathieu Desnoyers a.k.a. Dr. Tracing */
 	if (strncmp(progname, "drtrace", 7) == 0 ||
 			strncmp("compudj", getenv("USER"), 7) == 0) {
 		MSG("%c[%d;%dmWelcome back Dr Tracing!%c[%dm\n", 27,1,33,27,0);
------------------------------

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 Catching up on miscellaneous string and comment fixes (from 5c73c59)

[David Goulet]

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

Merged thanks!

David

On 12-01-30 03:27 PM, Thibault, Daniel wrote:
>    As promised, here is the first of a new set of patches, starting from the 2012-01-30 18:27 commit (2.0-pre18+-5c73c59).
> 
>    This first patch fixes miscellaneous little things wrong with strings and comments.
> 
> ------------------------------
> From cb6916eb52786f8caf1612c576500ca6bb6f4e76 Mon, 30 Jan 2012 15:20:12 -0500
> From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
> Date: Mon, 30 Jan 2012 15:19:50 -0500
> Subject: [PATCH] lttng-tools Catching up on miscellaneous string and comment fixes (from 5c73c59)
> 
> Signed-off-by: Daniel U. Thibault <daniel.thibault at drdc-rddc.gc.ca>
> 
> diff --git a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng-consumerd/lttng-consumerd.c b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng-consumerd/lttng-consumerd.c
> index 5d1ad06..7e3bf3c 100644
> --- a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng-consumerd/lttng-consumerd.c
> +++ b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng-consumerd/lttng-consumerd.c
> @@ -49,7 +49,7 @@
>  
>  #include "lttng-consumerd.h"
>  
> -/* TODO : support UST (all direct kern-ctl accesses). */
> +/* TODO : support UST (all direct kernel-ctl accesses). */
>  
>  /* the two threads (receive fd and poll) */
>  static pthread_t threads[2];
> @@ -310,7 +310,7 @@
>  	ret = lttcomm_connect_unix_sock(error_sock_path);
>  	/* not a fatal error, but all communication with lttng-sessiond will fail */
>  	if (ret < 0) {
> -		WARN("Cannot connect to error socket (is lttng-sessiond started ?)");
> +		WARN("Cannot connect to error socket (is lttng-sessiond started?)");
>  	}
>  	lttng_consumer_set_error_sock(ctx, ret);
>  
> diff --git a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng-sessiond/main.c b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng-sessiond/main.c
> index 9b2d1fe..8304de8 100644
> --- a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng-sessiond/main.c
> +++ b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng-sessiond/main.c
> @@ -314,7 +314,7 @@
>  static void teardown_kernel_session(struct ltt_session *session)
>  {
>  	if (!session->kernel_session) {
> -		DBG3("No kernel session when tearingdown session");
> +		DBG3("No kernel session when tearing down session");
>  		return;
>  	}
>  
> @@ -340,7 +340,7 @@
>  	int ret;
>  
>  	if (!session->ust_session) {
> -		DBG3("No UST session when tearingdown session");
> +		DBG3("No UST session when tearing down session");
>  		return;
>  	}
>  
> @@ -397,7 +397,7 @@
>  	}
>  	free(cmd);
>  
> -	DBG("Cleaning up all session");
> +	DBG("Cleaning up all sessions");
>  
>  	/* Destroy session list mutex */
>  	if (session_list_ptr != NULL) {
> diff --git a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/lttng.c b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/lttng.c
> index 31b4714..9c34143 100644
> --- a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/lttng.c
> +++ b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/lttng.c
> @@ -85,14 +85,14 @@
>  	fprintf(ofp, "usage: lttng [OPTIONS] <COMMAND>\n");
>  	fprintf(ofp, "\n");
>  	fprintf(ofp, "Options:\n");
> -	fprintf(ofp, "  -h, --help             Show this help\n");
> -	fprintf(ofp, "      --list-options     Simple listing of lttng options\n");
> -	fprintf(ofp, "      --list-commands    Simple listing of lttng commands\n");
> -	fprintf(ofp, "  -v, --verbose          Increase verbosity\n");
> -	fprintf(ofp, "  -q, --quiet            Quiet mode\n");
> -	fprintf(ofp, "  -g, --group NAME       Unix tracing group name. (default: tracing)\n");
> -	fprintf(ofp, "  -n, --no-sessiond      Don't spawn a session daemon\n");
> -	fprintf(ofp, "      --sessiond-path    Session daemon full path\n");
> +	fprintf(ofp, "  -h, --help               Show this help\n");
> +	fprintf(ofp, "      --list-options       Simple listing of lttng options\n");
> +	fprintf(ofp, "      --list-commands      Simple listing of lttng commands\n");
> +	fprintf(ofp, "  -v, --verbose            Increase verbosity\n");
> +	fprintf(ofp, "  -q, --quiet              Quiet mode\n");
> +	fprintf(ofp, "  -g, --group NAME         Unix tracing group name. (default: tracing)\n");
> +	fprintf(ofp, "  -n, --no-sessiond        Don't spawn a session daemon\n");
> +	fprintf(ofp, "      --sessiond-path PATH Session daemon full path\n");
>  	fprintf(ofp, "\n");
>  	fprintf(ofp, "Commands:\n");
>  	fprintf(ofp, "    add-context     Add context to event and/or channel\n");
> @@ -108,6 +108,8 @@
>  	fprintf(ofp, "    start           Start tracing\n");
>  	fprintf(ofp, "    stop            Stop tracing\n");
>  	fprintf(ofp, "    version         Show version information\n");
> +	fprintf(ofp, "\n");
> +	fprintf(ofp, "Each command also has its own -h, --help option.\n");
>  	fprintf(ofp, "\n");
>  	fprintf(ofp, "Please see the lttng(1) man page for full documentation.\n");
>  	fprintf(ofp, "See http://lttng.org for updates, bug reports and news.\n");
> @@ -176,11 +178,11 @@
>  
>  	switch (sig) {
>  		case SIGTERM:
> -			DBG("SIGTERM caugth");
> +			DBG("SIGTERM caught");
>  			clean_exit(EXIT_FAILURE);
>  			break;
>  		case SIGCHLD:
> -			DBG("SIGCHLD caugth");
> +			DBG("SIGCHLD caught");
>  			waitpid(sessiond_pid, &status, 0);
>  			recv_child_signal = 1;
>  			/* Indicate that the session daemon died */
> @@ -190,10 +192,10 @@
>  		case SIGUSR1:
>  			/* Notify is done */
>  			recv_child_signal = 1;
> -			DBG("SIGUSR1 caugth");
> +			DBG("SIGUSR1 caught");
>  			break;
>  		default:
> -			DBG("Unknown signal %d caugth", sig);
> +			DBG("Unknown signal %d caught", sig);
>  			break;
>  	}
>  
> @@ -514,7 +516,7 @@
>  
>  	progname = argv[0] ? argv[0] : "lttng";
>  
> -	/* For Mathieu Desnoyers aka Dr Tracing */
> +	/* For Mathieu Desnoyers a.k.a. Dr. Tracing */
>  	if (strncmp(progname, "drtrace", 7) == 0 ||
>  			strncmp("compudj", getenv("USER"), 7) == 0) {
>  		MSG("%c[%d;%dmWelcome back Dr Tracing!%c[%dm\n", 27,1,33,27,0);
> ------------------------------
> 
> 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/>
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iQEcBAEBAgAGBQJPJwViAAoJEELoaioR9I026XQH/idIWM7I6QjuPRbdTXgi1kQF
HdEYpjP9KOqLXtxfwGAPQUO/IFsqXRJU6+fWvuGHa76XJXIrQUFo8mCVeTSMaSDB
kxtdt+PJps67scsInNR04quKrnbX7/m5b3vMrZdmKeFgyZniRLIT7ZhYMi9fsaZJ
Hbq9pCheCnbr+xGKfhB8GR4orrRcxGT2F0hNWYXkg4SpaLaHMSP5bv7k/w9jkbus
lVlF3UKJmo5uO0XnxIKaruLDZr4RcvRuzamJ6ABMr6Khmo3RH8XRxmAdSULBY7oX
7rs0j1VvsUvSf6Upzapx6RWCuVyBlge3zHb24GJKPZxqjVHJRBIQq11U22ZrN+s=
=aamT
-----END PGP SIGNATURE-----

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

[Thibault, Daniel]

   This second patch fixes a memory leak in lttng-consumerd.c, various strings and comments, directs usage to stdout for --help, and enforces the return values of cmd_add_context().

------------------------------
From 39726bfddd9dc1cd8931eb1bbddc54b78c2d16e3 Mon, 30 Jan 2012 15:22:28 -0500
From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
Date: Mon, 30 Jan 2012 15:22:18 -0500
Subject: [PATCH] lttng-tools add_context.c : Fixing memory leaks

Signed-off-by: Daniel U. Thibault <daniel.thibault at drdc-rddc.gc.ca>

diff --git a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/commands/add_context.c b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/commands/add_context.c
index 2f3bce1..523a8b6 100644
--- a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/commands/add_context.c
+++ b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/commands/add_context.c
@@ -313,14 +313,14 @@
 	fprintf(ofp, "Options:\n");
 	fprintf(ofp, "  -h, --help               Show this help\n");
 	fprintf(ofp, "      --list-options       Simple listing of options\n");
-	fprintf(ofp, "  -s, --session NAME       Apply on session name\n");
-	fprintf(ofp, "  -c, --channel NAME       Apply on channel\n");
-	fprintf(ofp, "  -e, --event NAME         Apply on event\n");
+	fprintf(ofp, "  -s, --session NAME       Apply to session\n");
+	fprintf(ofp, "  -c, --channel NAME       Apply to channel\n");
+	fprintf(ofp, "  -e, --event NAME         Apply to event\n");
 	fprintf(ofp, "  -k, --kernel             Apply to the kernel tracer\n");
 #if 0
 	fprintf(ofp, "  -u, --userspace [CMD]    Apply to 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, "                           otherwise the domain is UST EXEC_NAME\n");
 	fprintf(ofp, "  -p, --pid PID            If -u, apply to specific PID (domain: UST PID)\n");
 #else
 	fprintf(ofp, "  -u, --userspace          Apply to the user-space tracer\n");
@@ -385,7 +385,7 @@
 		goto error;
 	}
 
-	/* Iterate over all context type given */
+	/* Iterate over all the context types given */
 	cds_list_for_each_entry(type, &ctx_type_list.head, list) {
 		context.ctx = type->opt->ctx_type;
 		if (context.ctx == LTTNG_EVENT_CONTEXT_PERF_COUNTER) {
@@ -444,7 +444,7 @@
 }
 
 /*
- * Add context on channel or event.
+ * Add context to channel or event.
  */
 int cmd_add_context(int argc, const char **argv)
 {
@@ -455,6 +455,7 @@
 
 	if (argc < 2) {
 		usage(stderr);
+		ret = CMD_ERROR;
 		goto end;
 	}
 
@@ -464,17 +465,10 @@
 	while ((opt = poptGetNextOpt(pc)) != -1) {
 		switch (opt) {
 		case OPT_HELP:
-			usage(stderr);
+			usage(stdout);
 			ret = CMD_SUCCESS;
 			goto end;
 		case OPT_TYPE:
-			type = malloc(sizeof(struct ctx_type));
-			if (type == NULL) {
-				perror("malloc ctx_type");
-				ret = -1;
-				goto end;
-			}
-
 			/*
 			 * Look up the index of opt_type in ctx_opts[] first, so we don't
 			 * have to free(type) on failure.
@@ -482,11 +476,21 @@
 			index = find_ctx_type_idx(opt_type);
 			if (index < 0) {
 				ERR("Unknown context type %s", opt_type);
+				ret = CMD_ERROR;
+				goto end;
+			}
+			type = malloc(sizeof(struct ctx_type));
+			if (type == NULL) {
+				perror("malloc ctx_type");
+				ret = CMD_FATAL;
 				goto end;
 			}
 			type->opt = &ctx_opts[index];
 			if (type->opt->ctx_type == -1) {
 				ERR("Unknown context type %s", opt_type);
+				free(type);
+				ret = CMD_ERROR;
+				goto end;
 			} else {
 				cds_list_add(&type->list, &ctx_type_list.head);
 			}
@@ -511,7 +515,7 @@
 	if (!opt_session_name) {
 		session_name = get_session_name();
 		if (session_name == NULL) {
-			ret = -1;
+			ret = CMD_ERROR;
 			goto end;
 		}
 	} else {
@@ -520,11 +524,11 @@
 
 	ret = add_context(session_name);
 
+end:
 	/* Cleanup allocated memory */
 	cds_list_for_each_entry_safe(type, tmptype, &ctx_type_list.head, list) {
 		free(type);
 	}
 
-end:
 	return ret;
 } 
------------------------------

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 add_context.c : Fixing memory leaks

[David Goulet]

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

Hi,

For some reason, this patch does not apply ...

Applying: lttng-tools add_context.c : Fixing memory leaks
error: patch failed: src/bin/lttng/commands/add_context.c:520
error: src/bin/lttng/commands/add_context.c: patch does not apply

In the meantime, I've merge the others since there are not using
lttng/commands/add_context.c

Also, this set of patch had a minor problem having
"lttng2-lttng-tools-2.0-pre18+-5c73c59/" added to all paths in the diff. Don't
know why because the previous one were fine...

Please update your git head before resending this one, I've merge your latest
working patches.

Thanks!
David

On 12-01-30 03:31 PM, Thibault, Daniel wrote:
>    This second patch fixes a memory leak in lttng-consumerd.c, various strings and comments, directs usage to stdout for --help, and enforces the return values of cmd_add_context().
> 
> ------------------------------
> From 39726bfddd9dc1cd8931eb1bbddc54b78c2d16e3 Mon, 30 Jan 2012 15:22:28 -0500
> From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
> Date: Mon, 30 Jan 2012 15:22:18 -0500
> Subject: [PATCH] lttng-tools add_context.c : Fixing memory leaks
> 
> Signed-off-by: Daniel U. Thibault <daniel.thibault at drdc-rddc.gc.ca>
> 
> diff --git a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/commands/add_context.c b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/commands/add_context.c
> index 2f3bce1..523a8b6 100644
> --- a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/commands/add_context.c
> +++ b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/commands/add_context.c
> @@ -313,14 +313,14 @@
>  	fprintf(ofp, "Options:\n");
>  	fprintf(ofp, "  -h, --help               Show this help\n");
>  	fprintf(ofp, "      --list-options       Simple listing of options\n");
> -	fprintf(ofp, "  -s, --session NAME       Apply on session name\n");
> -	fprintf(ofp, "  -c, --channel NAME       Apply on channel\n");
> -	fprintf(ofp, "  -e, --event NAME         Apply on event\n");
> +	fprintf(ofp, "  -s, --session NAME       Apply to session\n");
> +	fprintf(ofp, "  -c, --channel NAME       Apply to channel\n");
> +	fprintf(ofp, "  -e, --event NAME         Apply to event\n");
>  	fprintf(ofp, "  -k, --kernel             Apply to the kernel tracer\n");
>  #if 0
>  	fprintf(ofp, "  -u, --userspace [CMD]    Apply to 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, "                           otherwise the domain is UST EXEC_NAME\n");
>  	fprintf(ofp, "  -p, --pid PID            If -u, apply to specific PID (domain: UST PID)\n");
>  #else
>  	fprintf(ofp, "  -u, --userspace          Apply to the user-space tracer\n");
> @@ -385,7 +385,7 @@
>  		goto error;
>  	}
>  
> -	/* Iterate over all context type given */
> +	/* Iterate over all the context types given */
>  	cds_list_for_each_entry(type, &ctx_type_list.head, list) {
>  		context.ctx = type->opt->ctx_type;
>  		if (context.ctx == LTTNG_EVENT_CONTEXT_PERF_COUNTER) {
> @@ -444,7 +444,7 @@
>  }
>  
>  /*
> - * Add context on channel or event.
> + * Add context to channel or event.
>   */
>  int cmd_add_context(int argc, const char **argv)
>  {
> @@ -455,6 +455,7 @@
>  
>  	if (argc < 2) {
>  		usage(stderr);
> +		ret = CMD_ERROR;
>  		goto end;
>  	}
>  
> @@ -464,17 +465,10 @@
>  	while ((opt = poptGetNextOpt(pc)) != -1) {
>  		switch (opt) {
>  		case OPT_HELP:
> -			usage(stderr);
> +			usage(stdout);
>  			ret = CMD_SUCCESS;
>  			goto end;
>  		case OPT_TYPE:
> -			type = malloc(sizeof(struct ctx_type));
> -			if (type == NULL) {
> -				perror("malloc ctx_type");
> -				ret = -1;
> -				goto end;
> -			}
> -
>  			/*
>  			 * Look up the index of opt_type in ctx_opts[] first, so we don't
>  			 * have to free(type) on failure.
> @@ -482,11 +476,21 @@
>  			index = find_ctx_type_idx(opt_type);
>  			if (index < 0) {
>  				ERR("Unknown context type %s", opt_type);
> +				ret = CMD_ERROR;
> +				goto end;
> +			}
> +			type = malloc(sizeof(struct ctx_type));
> +			if (type == NULL) {
> +				perror("malloc ctx_type");
> +				ret = CMD_FATAL;
>  				goto end;
>  			}
>  			type->opt = &ctx_opts[index];
>  			if (type->opt->ctx_type == -1) {
>  				ERR("Unknown context type %s", opt_type);
> +				free(type);
> +				ret = CMD_ERROR;
> +				goto end;
>  			} else {
>  				cds_list_add(&type->list, &ctx_type_list.head);
>  			}
> @@ -511,7 +515,7 @@
>  	if (!opt_session_name) {
>  		session_name = get_session_name();
>  		if (session_name == NULL) {
> -			ret = -1;
> +			ret = CMD_ERROR;
>  			goto end;
>  		}
>  	} else {
> @@ -520,11 +524,11 @@
>  
>  	ret = add_context(session_name);
>  
> +end:
>  	/* Cleanup allocated memory */
>  	cds_list_for_each_entry_safe(type, tmptype, &ctx_type_list.head, list) {
>  		free(type);
>  	}
>  
> -end:
>  	return ret;
>  } 
> ------------------------------
> 
> 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/>
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iQEcBAEBAgAGBQJPJwkaAAoJEELoaioR9I02aQoH/0EYiVQzx860wWFh1gZ5iKCh
vXu5pbRqHNjxM7/Qn5PQHDmqzfSxT+NfBf8Azsm2jUnOOssb+27cUdMOQqCZfLRH
nEyfjb+mQlqYqIkEAaPQ/eAO1rabNYIzKw6nkEWBmJuzaL8TqS0SniIOvd1AzYp/
WRviRYnnENKjOs5IF28FA5Y6X0o1s0AfoDq8G/RZBF4x/2kHUhzLFlfbPGdzoHHv
zHrywCsQjBg++sA/+Mw+OdXSaxmQDI/S0YI7/GGCvx1p9407jO9aUPDr5bD99jxa
5vTcQRJjim/XYeyZMVQG2fmUoee/K0GE8v97PFn0qnyGfgM5n6nV3BDG774xwoM=
=lrUx
-----END PGP SIGNATURE-----

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

[Thibault, Daniel]

-----Message d'origine-----
Envoy??: 30 janvier 2012 16:18

> For some reason, this patch does not apply ...
>
> Applying: lttng-tools add_context.c : Fixing memory leaks
> error: patch failed: src/bin/lttng/commands/add_context.c:520
> error: src/bin/lttng/commands/add_context.c: patch does not apply
>
> In the meantime, I've merge the others since there are not using
> lttng/commands/add_context.c
>
> Also, this set of patch had a minor problem having
> "lttng2-lttng-tools-2.0-pre18+-5c73c59/" added to all paths in the diff. Don't
> know why because the previous one were fine...
>
> Please update your git head before resending this one, I've merge your latest
> working patches.

   Concerning the added "lttng2-lttng-tools-2.0-pre18+-5c73c59/" path fragment, that's my bad: I was manually editing that out of last week's patches.

   As for the patch not applying, I guess we're getting out of sync again.  But now that I've been told what the magic words Eclipse expects are (thanks Alexandre!), I should be able to get this back on track.

   Meanwhile I've got one more little patch ready so I might as well send it out: it simplifies the copy_string() calls of lttng_enable_event() and lttng_disable_event().

------------------------------
From 7d6f525e082259466cd58ef27670a40944445c65 Mon, 30 Jan 2012 16:29:56 -0500
From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
Date: Mon, 30 Jan 2012 16:29:44 -0500
Subject: [PATCH] lttng-ctl : Simplify copy_string() of lttng_enable_event() and lttng_disable_event()

Signed-off-by: Daniel U. Thibault <daniel.thibault at drdc-rddc.gc.ca>

diff --git a/src/lib/lttng-ctl/lttng-ctl.c b/src/lib/lttng-ctl/lttng-ctl.c
index 65aa3e3..fa901c7 100644
--- a/src/lib/lttng-ctl/lttng-ctl.c
+++ b/src/lib/lttng-ctl/lttng-ctl.c
@@ -515,13 +515,9 @@
 	}
 
 	/* If no channel name, we put the default name */
-	if (channel_name == NULL) {
-		copy_string(lsm.u.enable.channel_name, DEFAULT_CHANNEL_NAME,
-				sizeof(lsm.u.enable.channel_name));
-	} else {
-		copy_string(lsm.u.enable.channel_name, channel_name,
-				sizeof(lsm.u.enable.channel_name));
-	}
+	copy_string(lsm.u.enable.channel_name,
+			(channel_name ? channel_name : DEFAULT_CHANNEL_NAME),
+			sizeof(lsm.u.enable.channel_name));
 
 	copy_lttng_domain(&lsm.domain, &handle->domain);
 
@@ -553,13 +549,9 @@
 		return -1;
 	}
 
-	if (channel_name) {
-		copy_string(lsm.u.disable.channel_name, channel_name,
-				sizeof(lsm.u.disable.channel_name));
-	} else {
-		copy_string(lsm.u.disable.channel_name, DEFAULT_CHANNEL_NAME,
-				sizeof(lsm.u.disable.channel_name));
-	}
+	copy_string(lsm.u.disable.channel_name,
+			(channel_name ? channel_name : DEFAULT_CHANNEL_NAME),
+			sizeof(lsm.u.disable.channel_name));
 
 	copy_lttng_domain(&lsm.domain, &handle->domain);
------------------------------

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 add_context.c : Fixing memory leaks

[David Goulet]

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

Those are already upstream ;)

David

On 12-01-30 04:39 PM, Thibault, Daniel wrote:
> -----Message d'origine-----
> Envoy? : 30 janvier 2012 16:18
> 
>> For some reason, this patch does not apply ...
>>
>> Applying: lttng-tools add_context.c : Fixing memory leaks
>> error: patch failed: src/bin/lttng/commands/add_context.c:520
>> error: src/bin/lttng/commands/add_context.c: patch does not apply
>>
>> In the meantime, I've merge the others since there are not using
>> lttng/commands/add_context.c
>>
>> Also, this set of patch had a minor problem having
>> "lttng2-lttng-tools-2.0-pre18+-5c73c59/" added to all paths in the diff. Don't
>> know why because the previous one were fine...
>>
>> Please update your git head before resending this one, I've merge your latest
>> working patches.
> 
>    Concerning the added "lttng2-lttng-tools-2.0-pre18+-5c73c59/" path fragment, that's my bad: I was manually editing that out of last week's patches.
> 
>    As for the patch not applying, I guess we're getting out of sync again.  But now that I've been told what the magic words Eclipse expects are (thanks Alexandre!), I should be able to get this back on track.
> 
>    Meanwhile I've got one more little patch ready so I might as well send it out: it simplifies the copy_string() calls of lttng_enable_event() and lttng_disable_event().
> 
> ------------------------------
> From 7d6f525e082259466cd58ef27670a40944445c65 Mon, 30 Jan 2012 16:29:56 -0500
> From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
> Date: Mon, 30 Jan 2012 16:29:44 -0500
> Subject: [PATCH] lttng-ctl : Simplify copy_string() of lttng_enable_event() and lttng_disable_event()
> 
> Signed-off-by: Daniel U. Thibault <daniel.thibault at drdc-rddc.gc.ca>
> 
> diff --git a/src/lib/lttng-ctl/lttng-ctl.c b/src/lib/lttng-ctl/lttng-ctl.c
> index 65aa3e3..fa901c7 100644
> --- a/src/lib/lttng-ctl/lttng-ctl.c
> +++ b/src/lib/lttng-ctl/lttng-ctl.c
> @@ -515,13 +515,9 @@
>  	}
>  
>  	/* If no channel name, we put the default name */
> -	if (channel_name == NULL) {
> -		copy_string(lsm.u.enable.channel_name, DEFAULT_CHANNEL_NAME,
> -				sizeof(lsm.u.enable.channel_name));
> -	} else {
> -		copy_string(lsm.u.enable.channel_name, channel_name,
> -				sizeof(lsm.u.enable.channel_name));
> -	}
> +	copy_string(lsm.u.enable.channel_name,
> +			(channel_name ? channel_name : DEFAULT_CHANNEL_NAME),
> +			sizeof(lsm.u.enable.channel_name));
>  
>  	copy_lttng_domain(&lsm.domain, &handle->domain);
>  
> @@ -553,13 +549,9 @@
>  		return -1;
>  	}
>  
> -	if (channel_name) {
> -		copy_string(lsm.u.disable.channel_name, channel_name,
> -				sizeof(lsm.u.disable.channel_name));
> -	} else {
> -		copy_string(lsm.u.disable.channel_name, DEFAULT_CHANNEL_NAME,
> -				sizeof(lsm.u.disable.channel_name));
> -	}
> +	copy_string(lsm.u.disable.channel_name,
> +			(channel_name ? channel_name : DEFAULT_CHANNEL_NAME),
> +			sizeof(lsm.u.disable.channel_name));
>  
>  	copy_lttng_domain(&lsm.domain, &handle->domain);
> ------------------------------
> 
> 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/>
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iQEcBAEBAgAGBQJPJw85AAoJEELoaioR9I021j0H+wVMMwBDxaciadKKTTTOloph
XmNQIvTAuFPzmv4CDkTsDRXQk2NSbixzKpRcVPlhIdfmdTPFEK8uPPfKqXibEOqk
bcmfOiSBCTwE/a40IQWJtwdgmZE7/cl7DH9zO097fNKc06i5gfRTFZ3kW70Nn+Kz
e0Z3jhqOeLhW0QgN70aWQ6yufMwvqWiqOkI/fF0rBTgCAb2OHKaGzviqJ2Jik2iV
HzsBwt94XKRyRusEQ7C38NHabnKctBJo2ZrXiCG6wk3FOH6Gl+vgjF57GAyJL/WT
F2qIETe8oiiD8P1zFxLlVJQLrd7kzyaZ8dz3UIE+VKGnjmgxnQr1Ge0XNtp2Fz8=
=o8g5
-----END PGP SIGNATURE-----

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

[Thibault, Daniel]

> For some reason, this patch does not apply ...

   Okay, my git repository is now properly connected, and below is the patch rewritten for the current commit.  Besides enforcing the return values and outputting --help to stdout, the problem lay with the instances of struct ctx_type *type being created but not assigned to the doubly-linked list under various error conditions.

------------------------------
From e8e3c1db9311fda3b01a9318e263ce55b476222f Mon, 30 Jan 2012 16:55:02 -0500
From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
Date: Mon, 30 Jan 2012 16:54:47 -0500
Subject: [PATCH] lttng-tools add_context : Fixing memory leaks

Signed-off-by: Daniel U. Thibault <daniel.thibault at drdc-rddc.gc.ca>

diff --git a/src/bin/lttng/commands/add_context.c b/src/bin/lttng/commands/add_context.c
index 2f3bce1..2fec520 100644
--- a/src/bin/lttng/commands/add_context.c
+++ b/src/bin/lttng/commands/add_context.c
@@ -313,14 +313,14 @@
 	fprintf(ofp, "Options:\n");
 	fprintf(ofp, "  -h, --help               Show this help\n");
 	fprintf(ofp, "      --list-options       Simple listing of options\n");
-	fprintf(ofp, "  -s, --session NAME       Apply on session name\n");
-	fprintf(ofp, "  -c, --channel NAME       Apply on channel\n");
-	fprintf(ofp, "  -e, --event NAME         Apply on event\n");
+	fprintf(ofp, "  -s, --session NAME       Apply to session name\n");
+	fprintf(ofp, "  -c, --channel NAME       Apply to channel\n");
+	fprintf(ofp, "  -e, --event NAME         Apply to event\n");
 	fprintf(ofp, "  -k, --kernel             Apply to the kernel tracer\n");
 #if 0
 	fprintf(ofp, "  -u, --userspace [CMD]    Apply to 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, "                           otherwise the domain is UST EXEC_NAME\n");
 	fprintf(ofp, "  -p, --pid PID            If -u, apply to specific PID (domain: UST PID)\n");
 #else
 	fprintf(ofp, "  -u, --userspace          Apply to the user-space tracer\n");
@@ -385,7 +385,7 @@
 		goto error;
 	}
 
-	/* Iterate over all context type given */
+	/* Iterate over all the context types given */
 	cds_list_for_each_entry(type, &ctx_type_list.head, list) {
 		context.ctx = type->opt->ctx_type;
 		if (context.ctx == LTTNG_EVENT_CONTEXT_PERF_COUNTER) {
@@ -444,7 +444,7 @@
 }
 
 /*
- * Add context on channel or event.
+ * Add context to channel or event.
  */
 int cmd_add_context(int argc, const char **argv)
 {
@@ -455,6 +455,7 @@
 
 	if (argc < 2) {
 		usage(stderr);
+		ret = CMD_ERROR;
 		goto end;
 	}
 
@@ -464,17 +465,10 @@
 	while ((opt = poptGetNextOpt(pc)) != -1) {
 		switch (opt) {
 		case OPT_HELP:
-			usage(stderr);
+			usage(stdout);
 			ret = CMD_SUCCESS;
 			goto end;
 		case OPT_TYPE:
-			type = malloc(sizeof(struct ctx_type));
-			if (type == NULL) {
-				perror("malloc ctx_type");
-				ret = -1;
-				goto end;
-			}
-
 			/*
 			 * Look up the index of opt_type in ctx_opts[] first, so we don't
 			 * have to free(type) on failure.
@@ -482,11 +476,21 @@
 			index = find_ctx_type_idx(opt_type);
 			if (index < 0) {
 				ERR("Unknown context type %s", opt_type);
+				ret = CMD_ERROR;
+				goto end;
+			}
+			type = malloc(sizeof(struct ctx_type));
+			if (type == NULL) {
+				perror("malloc ctx_type");
+				ret = CMD_FATAL;
 				goto end;
 			}
 			type->opt = &ctx_opts[index];
 			if (type->opt->ctx_type == -1) {
 				ERR("Unknown context type %s", opt_type);
+				free(type);
+				ret = CMD_ERROR;
+				goto end;
 			} else {
 				cds_list_add(&type->list, &ctx_type_list.head);
 			}
@@ -511,7 +515,7 @@
 	if (!opt_session_name) {
 		session_name = get_session_name();
 		if (session_name == NULL) {
-			ret = -1;
+			ret = CMD_ERROR;
 			goto end;
 		}
 	} else {
@@ -520,11 +524,11 @@
 
 	ret = add_context(session_name);
 
+end:
 	/* Cleanup allocated memory */
 	cds_list_for_each_entry_safe(type, tmptype, &ctx_type_list.head, list) {
 		free(type);
 	}
 
-end:
 	return ret;
 }
------------------------------

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 add_context.c : Fixing memory leaks

[David Goulet]

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

Finally merged! :)

Thanks
David

On 12-01-30 04:58 PM, Thibault, Daniel wrote:
>> For some reason, this patch does not apply ...
> 
>    Okay, my git repository is now properly connected, and below is the patch rewritten for the current commit.  Besides enforcing the return values and outputting --help to stdout, the problem lay with the instances of struct ctx_type *type being created but not assigned to the doubly-linked list under various error conditions.
> 
> ------------------------------
> From e8e3c1db9311fda3b01a9318e263ce55b476222f Mon, 30 Jan 2012 16:55:02 -0500
> From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
> Date: Mon, 30 Jan 2012 16:54:47 -0500
> Subject: [PATCH] lttng-tools add_context : Fixing memory leaks
> 
> Signed-off-by: Daniel U. Thibault <daniel.thibault at drdc-rddc.gc.ca>
> 
> diff --git a/src/bin/lttng/commands/add_context.c b/src/bin/lttng/commands/add_context.c
> index 2f3bce1..2fec520 100644
> --- a/src/bin/lttng/commands/add_context.c
> +++ b/src/bin/lttng/commands/add_context.c
> @@ -313,14 +313,14 @@
>  	fprintf(ofp, "Options:\n");
>  	fprintf(ofp, "  -h, --help               Show this help\n");
>  	fprintf(ofp, "      --list-options       Simple listing of options\n");
> -	fprintf(ofp, "  -s, --session NAME       Apply on session name\n");
> -	fprintf(ofp, "  -c, --channel NAME       Apply on channel\n");
> -	fprintf(ofp, "  -e, --event NAME         Apply on event\n");
> +	fprintf(ofp, "  -s, --session NAME       Apply to session name\n");
> +	fprintf(ofp, "  -c, --channel NAME       Apply to channel\n");
> +	fprintf(ofp, "  -e, --event NAME         Apply to event\n");
>  	fprintf(ofp, "  -k, --kernel             Apply to the kernel tracer\n");
>  #if 0
>  	fprintf(ofp, "  -u, --userspace [CMD]    Apply to 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, "                           otherwise the domain is UST EXEC_NAME\n");
>  	fprintf(ofp, "  -p, --pid PID            If -u, apply to specific PID (domain: UST PID)\n");
>  #else
>  	fprintf(ofp, "  -u, --userspace          Apply to the user-space tracer\n");
> @@ -385,7 +385,7 @@
>  		goto error;
>  	}
>  
> -	/* Iterate over all context type given */
> +	/* Iterate over all the context types given */
>  	cds_list_for_each_entry(type, &ctx_type_list.head, list) {
>  		context.ctx = type->opt->ctx_type;
>  		if (context.ctx == LTTNG_EVENT_CONTEXT_PERF_COUNTER) {
> @@ -444,7 +444,7 @@
>  }
>  
>  /*
> - * Add context on channel or event.
> + * Add context to channel or event.
>   */
>  int cmd_add_context(int argc, const char **argv)
>  {
> @@ -455,6 +455,7 @@
>  
>  	if (argc < 2) {
>  		usage(stderr);
> +		ret = CMD_ERROR;
>  		goto end;
>  	}
>  
> @@ -464,17 +465,10 @@
>  	while ((opt = poptGetNextOpt(pc)) != -1) {
>  		switch (opt) {
>  		case OPT_HELP:
> -			usage(stderr);
> +			usage(stdout);
>  			ret = CMD_SUCCESS;
>  			goto end;
>  		case OPT_TYPE:
> -			type = malloc(sizeof(struct ctx_type));
> -			if (type == NULL) {
> -				perror("malloc ctx_type");
> -				ret = -1;
> -				goto end;
> -			}
> -
>  			/*
>  			 * Look up the index of opt_type in ctx_opts[] first, so we don't
>  			 * have to free(type) on failure.
> @@ -482,11 +476,21 @@
>  			index = find_ctx_type_idx(opt_type);
>  			if (index < 0) {
>  				ERR("Unknown context type %s", opt_type);
> +				ret = CMD_ERROR;
> +				goto end;
> +			}
> +			type = malloc(sizeof(struct ctx_type));
> +			if (type == NULL) {
> +				perror("malloc ctx_type");
> +				ret = CMD_FATAL;
>  				goto end;
>  			}
>  			type->opt = &ctx_opts[index];
>  			if (type->opt->ctx_type == -1) {
>  				ERR("Unknown context type %s", opt_type);
> +				free(type);
> +				ret = CMD_ERROR;
> +				goto end;
>  			} else {
>  				cds_list_add(&type->list, &ctx_type_list.head);
>  			}
> @@ -511,7 +515,7 @@
>  	if (!opt_session_name) {
>  		session_name = get_session_name();
>  		if (session_name == NULL) {
> -			ret = -1;
> +			ret = CMD_ERROR;
>  			goto end;
>  		}
>  	} else {
> @@ -520,11 +524,11 @@
>  
>  	ret = add_context(session_name);
>  
> +end:
>  	/* Cleanup allocated memory */
>  	cds_list_for_each_entry_safe(type, tmptype, &ctx_type_list.head, list) {
>  		free(type);
>  	}
>  
> -end:
>  	return ret;
>  }
> ------------------------------
> 
> 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/>
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iQEcBAEBAgAGBQJPJxRDAAoJEELoaioR9I02NscH/iWpn2bA8y2FOz8vR6sTXRhG
awO64UvWHxzkX0FX433Qa0qsZSgx4QGs7vmrQtf1/iI9o7LyEJ34Z9C36udeLXWc
MMldCf2LaEvUB+tM6nEkWS2+rIDFwm7HNSqndFsSLB13fdHcKci0H4I4WCxgBDbZ
J07pZC4/chcfSv/a9N9/c4NDgNzidF+vg8b8F9Y0YzP/w6Elji38muspiXheHii3
X1+nyjr8LIOZUf+aBLPetTmc+6nRKBpzaQSzLHk/8BYvLm068jS/rnER9AQNUvBJ
YzQ4UlopEn5SmCegKPBlYNp1mB3F+6+kA/1S7YwgvhnU787YCXPbAhoZZP9rEtI=
=KRBc
-----END PGP SIGNATURE-----

[lttng-dev] [PATCH] lttng-tools lttng-ctl, calibrate and create : Document return values

[Thibault, Daniel]

   This third patch documents (and in one instance, enforces) return values for functions of lttng-ctl, calibrate and create.

   It also fixes the test of getgroups(), which cannot return (grp_id < -1).

------------------------------
From 6739ca025c89937cd4e37c933715532fdc39ced0 Mon, 30 Jan 2012 15:54:01 -0500
From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
Date: Mon, 30 Jan 2012 15:53:49 -0500
Subject: [PATCH] lttng-tools lttng-ctl, calibrate and create : Document return values

Signed-off-by: Daniel U. Thibault <daniel.thibault at drdc-rddc.gc.ca>

diff --git a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/commands/calibrate.c b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/commands/calibrate.c
index a3c7b52..a60cadf 100644
--- a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/commands/calibrate.c
+++ b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/commands/calibrate.c
@@ -182,6 +182,8 @@
 
 /*
  * Calibrate LTTng tracer.
+ *
+ * Returns a CMD_* error.
  */
 int cmd_calibrate(int argc, const char **argv)
 {
diff --git a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/commands/create.c b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/commands/create.c
index 34754c7..9dc3752 100644
--- a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/commands/create.c
+++ b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/commands/create.c
@@ -52,17 +52,18 @@
 {
 	fprintf(ofp, "usage: lttng create [options] [NAME]\n");
 	fprintf(ofp, "\n");
+	fprintf(ofp, "  The default NAME is 'auto-yyyymmdd-hhmmss'\n");
 	fprintf(ofp, "  -h, --help           Show this help\n");
-	fprintf(ofp, "      --list-options       Simple listing of options\n");
+	fprintf(ofp, "      --list-options   Simple listing of options\n");
 	fprintf(ofp, "  -o, --output PATH    Specify output path for traces\n");
 	fprintf(ofp, "\n");
 }
 
 /*
- *  create_session
+ *  Create a tracing session.
+ *  If no name is specified, a default name is generated.
  *
- *  Create a tracing session. If no name specified, a default name will be
- *  generated.
+ *  Returns one of the CMD_* result constants.
  */
 static int create_session()
 {
@@ -147,9 +148,9 @@
 }
 
 /*
- *  cmd_create
- *
  *  The 'create <options>' first level command
+ *
+ *  Returns one of the CMD_* result constants.
  */
 int cmd_create(int argc, const char **argv)
 {
diff --git a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/lib/lttng-ctl/lttng-ctl.c b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/lib/lttng-ctl/lttng-ctl.c
index 6cc04ef..537934f 100644
--- a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/lib/lttng-ctl/lttng-ctl.c
+++ b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/lib/lttng-ctl/lttng-ctl.c
@@ -83,8 +83,8 @@
 /*
  * Send lttcomm_session_msg to the session daemon.
  *
- * On success, return 0
- * On error, return error code
+ * On success, returns the number of bytes sent (>=0)
+ * On error, returns -1
  */
 static int send_session_msg(struct lttcomm_session_msg *lsm)
 {
@@ -105,8 +105,8 @@
 /*
  * Receive data from the sessiond socket.
  *
- * On success, return 0
- * On error, return recv() error code
+ * On success, returns the number of bytes received (>=0)
+ * On error, returns -1 (recvmsg() error) or -ENOTCONN
  */
 static int recv_data_sessiond(void *buf, size_t len)
 {
@@ -124,7 +124,7 @@
 }
 
 /*
- *  Check if the specified group name exist.
+ *  Check if we are in the specified group.
  *
  *  If yes return 1, else return -1.
  */
@@ -153,13 +153,14 @@
 	}
 
 	/* Alloc group list of the right size */
+	/* Note that malloc(0) will return a valid pointer */
 	grp_list = malloc(grp_list_size * sizeof(gid_t));
 	if (!grp_list) {
-		ret = -1;
+		perror("malloc");
 		goto end;
 	}
 	grp_id = getgroups(grp_list_size, grp_list);
-	if (grp_id < -1) {
+	if (grp_id < 0) {
 		perror("getgroups");
 		goto free_list;
 	}
@@ -209,8 +210,10 @@
 }
 
 /*
- * Set sessiond socket path by putting it in the global sessiond_sock_path
- * variable.
+ * Set sessiond socket path by putting it in the global
+ * sessiond_sock_path variable.
+ * Returns 0 on success,
+ * -ENOMEM on failure (the sessiond socket path is somehow too long)
  */
 static int set_session_daemon_path(void)
 {
@@ -268,7 +271,7 @@
 
 	ret = set_session_daemon_path();
 	if (ret < 0) {
-		return ret;
+		return -1; /* set_session_daemon_path() returns -ENOMEM */
 	}
 
 	/* Connect to the sesssion daemon */
@@ -284,7 +287,8 @@
 }
 
 /*
- *  Clean disconnect the session daemon.
+ *  Clean disconnect from the session daemon.
+ *  On success, return 0. On error, return -1.
  */
 static int disconnect_sessiond(void)
 {
@@ -373,6 +377,7 @@
 
 /*
  * Create lttng handle and return pointer.
+ * The returned pointer will be NULL in case of malloc() error.
  */
 struct lttng_handle *lttng_create_handle(const char *session_name,
 		struct lttng_domain *domain)
@@ -408,6 +413,7 @@
 
 /*
  * Register an outside consumer.
+ * Returns size of returned session payload data or a negative error code.
  */
 int lttng_register_consumer(struct lttng_handle *handle,
 		const char *socket_path)
@@ -425,7 +431,8 @@
 }
 
 /*
- *  Start tracing for all trace of the session.
+ *  Start tracing for all traces of the session.
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_start_tracing(const char *session_name)
 {
@@ -443,7 +450,8 @@
 }
 
 /*
- *  Stop tracing for all trace of the session.
+ *  Stop tracing for all traces of the session.
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_stop_tracing(const char *session_name)
 {
@@ -496,7 +504,10 @@
 }
 
 /*
- * Enable event
+ *  Enable event(s) for a channel.
+ *  If no event name is specified, all events are enabled.
+ *  If no channel name is specified, the default 'channel0' is used.
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_enable_event(struct lttng_handle *handle,
 		struct lttng_event *ev, const char *channel_name)
@@ -532,7 +543,10 @@
 }
 
 /*
- * Disable event of a channel and domain.
+ *  Disable event(s) of a channel and domain.
+ *  If no event name is specified, all events are disabled.
+ *  If no channel name is specified, the default 'channel0' is used.
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_disable_event(struct lttng_handle *handle, const char *name,
 		const char *channel_name)
@@ -567,7 +581,8 @@
 }
 
 /*
- * Enable channel per domain
+ *  Enable channel per domain
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_enable_channel(struct lttng_handle *handle,
 		struct lttng_channel *chan)
@@ -594,7 +609,8 @@
 }
 
 /*
- * All tracing will be stopped for registered events of the channel.
+ *  All tracing will be stopped for registered events of the channel.
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_disable_channel(struct lttng_handle *handle, const char *name)
 {
@@ -619,10 +635,10 @@
 }
 
 /*
- * List all available tracepoints of domain.
- *
- * Return the size (bytes) of the list and set the events array.
- * On error, return negative value.
+ *  Lists all available tracepoints of domain.
+ *  Sets the contents of the events array.
+ *  Returns the number of lttng_event entries in events;
+ *  on error, returns a negative value.
  */
 int lttng_list_tracepoints(struct lttng_handle *handle,
 		struct lttng_event **events)
@@ -646,10 +662,12 @@
 }
 
 /*
- *  Return a human readable string of code
+ *  Returns a human readable string describing
+ *  the error code (a negative value).
  */
 const char *lttng_strerror(int code)
 {
+	/* lttcomm error codes range from -LTTCOMM_OK down to -LTTCOMM_NR */
 	if (code > -LTTCOMM_OK) {
 		return "Ended with errors";
 	}
@@ -658,7 +676,8 @@
 }
 
 /*
- *  Create a brand new session using name.
+ *  Create a brand new session using name and path.
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_create_session(const char *name, const char *path)
 {
@@ -673,6 +692,7 @@
 
 /*
  *  Destroy session using name.
+ *  Returns size of returned session payload data or a negative error code.
  */
 int lttng_destroy_session(const char *session_name)
 {
@@ -691,9 +711,9 @@
 
 /*
  *  Ask the session daemon for all available sessions.
- *
- *  Return number of session.
- *  On error, return negative value.
+ *  Sets the contents of the sessions array.
+ *  Returns the number of lttng_session entries in sessions;
+ *  on error, returns a negative value.
  */
 int lttng_list_sessions(struct lttng_session **sessions)
 {
@@ -710,7 +730,10 @@
 }
 
 /*
- * List domain of a session.
+ *  Ask the session daemon for all available domains of a session.
+ *  Sets the contents of the domains array.
+ *  Returns the number of lttng_domain entries in domains;
+ *  on error, returns a negative value.
  */
 int lttng_list_domains(const char *session_name,
 		struct lttng_domain **domains)
@@ -735,7 +758,10 @@
 }
 
 /*
- * List channels of a session
+ *  Ask the session daemon for all available channels of a session.
+ *  Sets the contents of the channels array.
+ *  Returns the number of lttng_channel entries in channels;
+ *  on error, returns a negative value.
  */
 int lttng_list_channels(struct lttng_handle *handle,
 		struct lttng_channel **channels)
@@ -762,7 +788,10 @@
 }
 
 /*
- * List events of a session channel.
+ *  Ask the session daemon for all available events of a session channel.
+ *  Sets the contents of the events array.
+ *  Returns the number of lttng_event entries in events;
+ *  on error, returns a negative value.
  */
 int lttng_list_events(struct lttng_handle *handle,
 		const char *channel_name, struct lttng_event **events)
@@ -792,8 +821,9 @@
 }
 
 /*
- * Set tracing group variable with name. This function allocate memory pointed
- * by tracing_group.
+ * Sets the tracing_group variable with name.
+ * This function allocates memory pointed to by tracing_group.
+ * On success, returns 0, on error, returns -1 (null name) or -ENOMEM.
  */
 int lttng_set_tracing_group(const char *name)
 {
@@ -831,6 +861,7 @@
 
 /*
  * Set default channel attributes.
+ * If either or both of the arguments are null, nothing happens.
  */
 void lttng_channel_set_default_attr(struct lttng_domain *domain,
 		struct lttng_channel_attr *attr)
@@ -875,7 +906,7 @@
  * Check if session daemon is alive.
  *
  * Return 1 if alive or 0 if not.
- * On error return -1
+ * On error returns a negative value.
  */
 int lttng_session_daemon_alive(void)
 {
------------------------------

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 : create.c : Simplify create_session()

[Thibault, Daniel]

   This fourth patch simplifies create_session() in create.c

------------------------------
From e81d91e548bc9dc94dc9e89ff0bf1150f43ebbda Mon, 30 Jan 2012 16:03:10 -0500
From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
Date: Mon, 30 Jan 2012 16:02:53 -0500
Subject: [PATCH] lttng-tools : create.c : Simplify create_session()

Signed-off-by: Daniel U. Thibault <daniel.thibault at drdc-rddc.gc.ca>

diff --git a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/commands/create.c b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/commands/create.c
index 9dc3752..de2d909 100644
--- a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/commands/create.c
+++ b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/bin/lttng/commands/create.c
@@ -67,7 +67,7 @@
  */
 static int create_session()
 {
-	int ret, have_name = 0;
+	int ret;
 	char datetime[16];
 	char *session_name, *traces_path = NULL, *alloc_path = NULL;
 	time_t rawtime;
@@ -80,37 +80,33 @@
 
 	/* Auto session name creation */
 	if (opt_session_name == NULL) {
-		ret = asprintf(&session_name, "auto-%s", datetime);
+		ret = asprintf(&session_name, "auto");
 		if (ret < 0) {
 			perror("asprintf session name");
+			ret = CMD_ERROR;
 			goto error;
 		}
 		DBG("Auto session name set to %s", session_name);
 	} else {
 		session_name = opt_session_name;
-		have_name = 1;
 	}
 
 	/* Auto output path */
 	if (opt_output_path == NULL) {
 		alloc_path = strdup(config_get_default_path());
 		if (alloc_path == NULL) {
-			ERR("Home path not found.\n \
-				 Please specify an output path using -o, --output PATH");
+			ERR("Home path not found.\n%s"
+				"Please specify an output path using -o, --output PATH\n");
 			ret = CMD_FATAL;
 			goto error;
 		}
 
-		if (have_name) {
-			ret = asprintf(&traces_path, "%s/" DEFAULT_TRACE_DIR_NAME
+		ret = asprintf(&traces_path, "%s/" DEFAULT_TRACE_DIR_NAME
 					"/%s-%s", alloc_path, session_name, datetime);
-		} else {
-			ret = asprintf(&traces_path, "%s/" DEFAULT_TRACE_DIR_NAME
-					"/%s", alloc_path, session_name);
-		}
 
 		if (ret < 0) {
 			perror("asprintf trace dir name");
+			ret = CMD_ERROR;
 			goto error;
 		}
 	} else {
------------------------------

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] Rewrites lttng-ctl's set_session_daemon_path(), fix snprintf() return value tests so the code works with both GNU C < 2.1 and >= 2.1

[Thibault, Daniel]

   This fifth patch rewrites lttng-ctl's set_session_daemon_path() to avoid duplicating snippets of code.  It also fixes the snprintf return value test so the code works with both GNU C < 2.1 and >= 2.1.  With GNU C < 2.1, snprintf returns -1 if the target buffer is too small; with GNU C >= 2.1, snprintf returns the required size (excluding the closing null) under the same conditions.

   I do believe it is functionally identical to the version it replaces, but someone should check to make sure.   :-)

------------------------------
From 3d90c8707c362c9ecf0836c25d500c818ba2e2f0 Mon, 30 Jan 2012 16:25:29 -0500
From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
Date: Mon, 30 Jan 2012 16:25:15 -0500
Subject: [PATCH] Rewrites lttng-ctl's set_session_daemon_path(), fix snprintf() return value tests so the code works with both GNU C < 2.1 and >= 2.1

Signed-off-by: Daniel U. Thibault <daniel.thibault at drdc-rddc.gc.ca>

diff --git a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/lib/lttng-ctl/lttng-ctl.c b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/lib/lttng-ctl/lttng-ctl.c
index 537934f..65aa3e3 100644
--- a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/lib/lttng-ctl/lttng-ctl.c
+++ b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/lib/lttng-ctl/lttng-ctl.c
@@ -228,35 +228,31 @@
 		in_tgroup = check_tracing_group(tracing_group);
 	}
 
-	if (uid == 0) {
-		/* Root */
+	if ((uid == 0) || in_tgroup) {
 		copy_string(sessiond_sock_path,
 				DEFAULT_GLOBAL_CLIENT_UNIX_SOCK,
 				sizeof(sessiond_sock_path));
-	} else if (in_tgroup) {
-		/* Tracing group */
-		copy_string(sessiond_sock_path,
-				DEFAULT_GLOBAL_CLIENT_UNIX_SOCK,
-				sizeof(sessiond_sock_path));
-
-		ret = try_connect_sessiond(sessiond_sock_path);
-		if (ret < 0) {
-			/* Global session daemon not available */
-			if (snprintf(sessiond_sock_path, sizeof(sessiond_sock_path),
-						DEFAULT_HOME_CLIENT_UNIX_SOCK,
-						getenv("HOME")) < 0) {
-				return -ENOMEM;
-			}
 		}
-	} else {
-		/* Not in tracing group and not root, default */
-		if (snprintf(sessiond_sock_path, PATH_MAX,
-					DEFAULT_HOME_CLIENT_UNIX_SOCK,
-					getenv("HOME")) < 0) {
+	if (uid != 0) {
+		if (in_tgroup) {
+			/* Tracing group */
+			ret = try_connect_sessiond(sessiond_sock_path);
+			if (ret >= 0) goto end;
+			/* Global session daemon not available... */
+		}
+		/* ...or not in tracing group (and not root), default */
+		/*
+		 * With GNU C <  2.1, snprintf returns -1 if the target buffer is too small;
+		 * With GNU C >= 2.1, snprintf returns the required size (excluding closing null)
+		 */
+		ret = snprintf(sessiond_sock_path, sizeof(sessiond_sock_path),
+				DEFAULT_HOME_CLIENT_UNIX_SOCK,
+				getenv("HOME"));
+		if ((ret < 0) || (ret >= sizeof(sessiond_sock_path))) {
 			return -ENOMEM;
 		}
 	}
-
+end:
 	return 0;
 }

------------------------------

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] Rewrites lttng-ctl's set_session_daemon_path(), fix snprintf() return value tests so the code works with both GNU C < 2.1 and >= 2.1

[David Goulet]

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

Merged thanks!

We did indeed validate the function and it's seems ok and test passes.

If you are changing good portion of code, you can run "make check" to start the
tests and you'll see if you break something. It's not very complete tests but
it's a start.

Cheers
David

On 12-01-30 04:26 PM, Thibault, Daniel wrote:
>    This fifth patch rewrites lttng-ctl's set_session_daemon_path() to avoid duplicating snippets of code.  It also fixes the snprintf return value test so the code works with both GNU C < 2.1 and >= 2.1.  With GNU C < 2.1, snprintf returns -1 if the target buffer is too small; with GNU C >= 2.1, snprintf returns the required size (excluding the closing null) under the same conditions.
> 
>    I do believe it is functionally identical to the version it replaces, but someone should check to make sure.   :-)
> 
> ------------------------------
> From 3d90c8707c362c9ecf0836c25d500c818ba2e2f0 Mon, 30 Jan 2012 16:25:29 -0500
> From: Daniel U. Thibault <daniel.thibault@drdc-rddc.gc.ca>
> Date: Mon, 30 Jan 2012 16:25:15 -0500
> Subject: [PATCH] Rewrites lttng-ctl's set_session_daemon_path(), fix snprintf() return value tests so the code works with both GNU C < 2.1 and >= 2.1
> 
> Signed-off-by: Daniel U. Thibault <daniel.thibault at drdc-rddc.gc.ca>
> 
> diff --git a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/lib/lttng-ctl/lttng-ctl.c b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/lib/lttng-ctl/lttng-ctl.c
> index 537934f..65aa3e3 100644
> --- a/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/lib/lttng-ctl/lttng-ctl.c
> +++ b/lttng2-lttng-tools-2.0-pre18+-5c73c59/src/lib/lttng-ctl/lttng-ctl.c
> @@ -228,35 +228,31 @@
>  		in_tgroup = check_tracing_group(tracing_group);
>  	}
>  
> -	if (uid == 0) {
> -		/* Root */
> +	if ((uid == 0) || in_tgroup) {
>  		copy_string(sessiond_sock_path,
>  				DEFAULT_GLOBAL_CLIENT_UNIX_SOCK,
>  				sizeof(sessiond_sock_path));
> -	} else if (in_tgroup) {
> -		/* Tracing group */
> -		copy_string(sessiond_sock_path,
> -				DEFAULT_GLOBAL_CLIENT_UNIX_SOCK,
> -				sizeof(sessiond_sock_path));
> -
> -		ret = try_connect_sessiond(sessiond_sock_path);
> -		if (ret < 0) {
> -			/* Global session daemon not available */
> -			if (snprintf(sessiond_sock_path, sizeof(sessiond_sock_path),
> -						DEFAULT_HOME_CLIENT_UNIX_SOCK,
> -						getenv("HOME")) < 0) {
> -				return -ENOMEM;
> -			}
>  		}
> -	} else {
> -		/* Not in tracing group and not root, default */
> -		if (snprintf(sessiond_sock_path, PATH_MAX,
> -					DEFAULT_HOME_CLIENT_UNIX_SOCK,
> -					getenv("HOME")) < 0) {
> +	if (uid != 0) {
> +		if (in_tgroup) {
> +			/* Tracing group */
> +			ret = try_connect_sessiond(sessiond_sock_path);
> +			if (ret >= 0) goto end;
> +			/* Global session daemon not available... */
> +		}
> +		/* ...or not in tracing group (and not root), default */
> +		/*
> +		 * With GNU C <  2.1, snprintf returns -1 if the target buffer is too small;
> +		 * With GNU C >= 2.1, snprintf returns the required size (excluding closing null)
> +		 */
> +		ret = snprintf(sessiond_sock_path, sizeof(sessiond_sock_path),
> +				DEFAULT_HOME_CLIENT_UNIX_SOCK,
> +				getenv("HOME"));
> +		if ((ret < 0) || (ret >= sizeof(sessiond_sock_path))) {
>  			return -ENOMEM;
>  		}
>  	}
> -
> +end:
>  	return 0;
>  }
> 
> ------------------------------
> 
> 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/>
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iQEcBAEBAgAGBQJPJw0UAAoJEELoaioR9I02eSsIAMjz/mUaOmPnK4pt187VIDin
ibA80F8irkILJfZMa1Vx2E4KsvDpAsmeVBlsf72A8otUsMNf72V0T+Vm2sRsXxK4
Xy4Y79QMaMplk1XgYcRZqTEIQear02SxG+rQMIo4R+gZxXWpyy+vC63mQoib67wE
v70w6t8nJ0q9cZasxUKu1iibu5GSGul7aQZi0naPJ2ODiFG6cXVUe0IBqhcF4RYe
7nXOauNLeUAMGuBHRetEzaXy3dr/F6ff1WctP2Smq0pRjKg5FGPg9T09uK2dVJAm
/ZLkcsF0dfK6fwppUG3Lof0M/p+ala/C9R62o0hwkdsPdckTnkfTYsE5J2ClxZw=
=89FN
-----END PGP SIGNATURE-----