[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