[lttng-dev] [PATCH] [RESUBMISSION] lttng-tools lib/lttng-ctl/lttng-ctl.c : Document return values and more
David Goulet
david.goulet at polymtl.ca
Mon Jan 30 13:11:44 EST 2012
-----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 at 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 at 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 at 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-----
More information about the lttng-dev
mailing list