From mboxrd@z Thu Jan 1 00:00:00 1970 From: david.goulet@polymtl.ca (David Goulet) Date: Fri, 27 Jan 2012 14:46:52 -0500 Subject: [lttng-dev] [PATCH] lttng-tools lib/lttng-ctl/lttng-ctl.c : Document return values and more In-Reply-To: <3CA1A65C03C1A34C81BBC6272782E69680895F@valcartierex01.valcartier.drdc-rddc.gc.ca> References: <3CA1A65C03C1A34C81BBC6272782E69680895F@valcartierex01.valcartier.drdc-rddc.gc.ca> Message-ID: <4F22FF2C.3060604@polymtl.ca> -----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 > 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 ' 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 > > > _______________________________________________ > 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-----