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

David Goulet david.goulet at polymtl.ca
Fri Jan 27 14:46:52 EST 2012


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



More information about the lttng-dev mailing list