[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
       [not found] <mailman.1.1327683602.12686.lttng-dev@lists.lttng.org>
@ 2012-01-27 18:41 ` Thibault, Daniel
  2012-01-27 19:46   ` David Goulet
  0 siblings, 1 reply; 19+ messages in thread
From: Thibault, Daniel @ 2012-01-27 18:41 UTC (permalink / raw)


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/>



^ permalink raw reply	[flat|nested] 19+ messages in thread