[lttng-dev] [PATCH] Fix alignment and wording of usage() output, setup some TODOs (Replacing all the deferred patches)
David Goulet
david.goulet at polymtl.ca
Wed Feb 1 18:37:03 EST 2012
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi Daniel,
I've made some fixes on error code handling across lttng cli code. So, to make
this patch worth it, I've cherry picked changes made to the usage wording and
made a separate commit mentionning you of course.
Comments below:
On 12-02-01 05:50 PM, Thibault, Daniel wrote:
> From 1b5ba907a4910bd10dda3fbd14c1f1751893e268 Wed, 1 Feb 2012 17:48:14 -0500
> From: Daniel U. Thibault <daniel.thibault at drdc-rddc.gc.ca>
> Date: Wed, 1 Feb 2012 17:47:18 -0500
> Subject: [PATCH] Fix alignment and wording of usage() output, setup some TODOs
>
> Signed-off-by: Daniel U. Thibault <daniel.thibault at drdc-rddc.gc.ca>
>
> diff --git a/src/bin/lttng/commands/calibrate.c b/src/bin/lttng/commands/calibrate.c
> index 547b349..2328e44 100644
> --- a/src/bin/lttng/commands/calibrate.c
> +++ b/src/bin/lttng/commands/calibrate.c
> @@ -101,15 +101,13 @@
> fprintf(ofp, "\n");
> fprintf(ofp, "Calibrate options:\n");
> #if 0
> - fprintf(ofp, " --tracepoint Tracepoint event (default)\n");
> - fprintf(ofp, " --probe\n");
> - fprintf(ofp, " Dynamic probe.\n");
> + fprintf(ofp, " --tracepoint Tracepoint event (default)\n");
> + fprintf(ofp, " --probe Dynamic probe.\n");
> #if 0
> - fprintf(ofp, " --function:entry symbol\n");
> - fprintf(ofp, " Function tracer event\n");
> + fprintf(ofp, " --function:entry symbol Function tracer event\n");
> #endif
> - fprintf(ofp, " --syscall System call eventl\n");
> - fprintf(ofp, " --marker User-space marker (deprecated)\n");
> + fprintf(ofp, " --syscall System call eventl\n");
> + fprintf(ofp, " --marker User-space marker (deprecated)\n");
> #else
> fprintf(ofp, " --function Dynamic function entry/return probe (default)\n");
> #endif
> @@ -127,6 +125,7 @@
> struct lttng_domain dom;
> struct lttng_calibrate calibrate;
>
> + /* TODO Enforce requirement of exactly one domain in cmd_...() */
Yes that would be a good idea actually. If you want to send me a patch for that,
I'll be happy.
> /* Create lttng domain */
> if (opt_kernel) {
> dom.type = LTTNG_DOMAIN_KERNEL;
> @@ -138,6 +137,7 @@
> goto error;
> }
>
> + /* TODO Upgrade this failure to CMD_FATAL? */
> handle = lttng_create_handle(NULL, &dom);
> if (handle == NULL) {
> ret = CMD_ERROR;
> @@ -172,6 +172,7 @@
> goto error;
> }
>
> + /* TODO Remove this unnecessary assignment */
> ret = CMD_SUCCESS;
>
> error:
> diff --git a/src/bin/lttng/commands/destroy.c b/src/bin/lttng/commands/destroy.c
> index b0262ea..6203c21 100644
> --- a/src/bin/lttng/commands/destroy.c
> +++ b/src/bin/lttng/commands/destroy.c
> @@ -52,7 +52,7 @@
> fprintf(ofp, "get it from the configuration directory (.lttng).\n");
> fprintf(ofp, "\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, "\n");
> }
>
> diff --git a/src/bin/lttng/commands/disable_channels.c b/src/bin/lttng/commands/disable_channels.c
> index 05ee0e6..423d867 100644
> --- a/src/bin/lttng/commands/disable_channels.c
> +++ b/src/bin/lttng/commands/disable_channels.c
> @@ -70,15 +70,15 @@
> fprintf(ofp, "\n");
> fprintf(ofp, " -h, --help Show this help\n");
> fprintf(ofp, " --list-options Simple listing of options\n");
> - fprintf(ofp, " -s, --session Apply on session name\n");
> - fprintf(ofp, " -k, --kernel Apply for the kernel tracer\n");
> + fprintf(ofp, " -s, --session NAME Apply to session name\n");
> + fprintf(ofp, " -k, --kernel Apply to the kernel tracer\n");
> #if 0
> - fprintf(ofp, " -u, --userspace [CMD] Apply for the user-space tracer\n");
> + fprintf(ofp, " -u, --userspace [CMD] Apply to the user-space tracer\n");
> fprintf(ofp, " If no CMD, the domain used is UST global\n");
> - fprintf(ofp, " or else the domain is UST EXEC_NAME\n");
> + fprintf(ofp, " otherwise the domain is UST EXEC_NAME\n");
> fprintf(ofp, " -p, --pid PID If -u, apply to specific PID (domain: UST PID)\n");
> #else
> - fprintf(ofp, " -u, --userspace Apply for the user-space tracer\n");
> + fprintf(ofp, " -u, --userspace Apply to the user-space tracer\n");
> #endif
> fprintf(ofp, "\n");
> }
> @@ -92,6 +92,7 @@
> char *channel_name;
> struct lttng_domain dom;
>
> + /* TODO Enforce requirement of exactly one domain in cmd_...() */
> /* Create lttng domain */
> if (opt_kernel) {
> dom.type = LTTNG_DOMAIN_KERNEL;
> @@ -102,7 +103,7 @@
> ret = CMD_ERROR;
> goto error;
> }
> -
> + /* TODO Document and enforce return values (e.g. -1 should be CMD_ERROR) */
You are right, CMD_FATAL should be returned here since at this point only malloc
can fail. We can change that now before the release.
> handle = lttng_create_handle(session_name, &dom);
> if (handle == NULL) {
> ret = -1;
> @@ -111,9 +112,12 @@
>
> /* Strip channel list */
> channel_name = strtok(opt_channels, ",");
> + /* TODO Issue CMD_WARNING if some lttng_disable_channel() calls fail */
> + /* TODO or if opt_channels is effectively empty */
I've done that in previous commit :).
> while (channel_name != NULL) {
> DBG("Disabling channel %s", channel_name);
>
> + /* TODO setup a chan_name[LTTNG_SYMBOL_NAME_LEN] filter */
> ret = lttng_disable_channel(handle, channel_name);
> if (ret < 0) {
> goto error;
> @@ -186,6 +190,7 @@
> ret = disable_channels(session_name);
>
> end:
> + /* TODO Fix memory leak of session_name when opt_session_name == NULL */
Fixed
> poptFreeContext(pc);
> return ret;
> }
> diff --git a/src/bin/lttng/commands/disable_events.c b/src/bin/lttng/commands/disable_events.c
> index 4ddcf54..98901db 100644
> --- a/src/bin/lttng/commands/disable_events.c
> +++ b/src/bin/lttng/commands/disable_events.c
> @@ -74,17 +74,17 @@
> fprintf(ofp, "\n");
> fprintf(ofp, " -h, --help Show this help\n");
> fprintf(ofp, " --list-options Simple listing of options\n");
> - fprintf(ofp, " -s, --session Apply on session name\n");
> - fprintf(ofp, " -c, --channel Apply on this channel\n");
> + fprintf(ofp, " -s, --session NAME Apply to session name\n");
> + fprintf(ofp, " -c, --channel NAME Apply to this channel\n");
> fprintf(ofp, " -a, --all-events Disable all tracepoints\n");
> - fprintf(ofp, " -k, --kernel Apply for the kernel tracer\n");
> + fprintf(ofp, " -k, --kernel Apply to the kernel tracer\n");
> #if 0
> fprintf(ofp, " -u, --userspace [CMD] Apply for the user-space tracer\n");
> fprintf(ofp, " If no CMD, the domain used is UST global\n");
> - fprintf(ofp, " or else the domain is UST EXEC_NAME\n");
> + fprintf(ofp, " otherwise the domain is UST EXEC_NAME\n");
> fprintf(ofp, " -p, --pid PID If -u, apply to specific PID (domain: UST PID)\n");
> #else
> - fprintf(ofp, " -u, --userspace Apply for the user-space tracer\n");
> + fprintf(ofp, " -u, --userspace Apply to the user-space tracer\n");
> #endif
> fprintf(ofp, "\n");
> }
> @@ -110,6 +110,7 @@
> channel_name = opt_channel_name;
> }
>
> + /* TODO Enforce requirement of exactly one domain in cmd_...() */
> if (opt_kernel && opt_userspace) {
> ERR("Can't use -k/--kernel and -u/--userspace together");
> ret = CMD_FATAL;
> @@ -127,6 +128,7 @@
> goto error;
> }
>
> + /* TODO Document and enforce return values (e.g. -1 should be CMD_FATAL) */
> handle = lttng_create_handle(session_name, &dom);
> if (handle == NULL) {
> ret = -1;
> @@ -146,6 +148,8 @@
>
> /* Strip event list */
> event_name = strtok(opt_event_list, ",");
> + /* TODO Issue CMD_WARNING if some lttng_disable_event() calls fail */
> + /* TODO or if opt_event_list is effectively empty */
> while (event_name != NULL) {
> /* Kernel tracer action */
> if (opt_kernel) {
> @@ -244,6 +248,7 @@
> ret = disable_events(session_name);
>
> end:
> + /* TODO Fix memory leak of session_name when opt_session_name == NULL */
> poptFreeContext(pc);
> return ret;
> }
> diff --git a/src/bin/lttng/commands/enable_channels.c b/src/bin/lttng/commands/enable_channels.c
> index 19b6b26..51f4248 100644
> --- a/src/bin/lttng/commands/enable_channels.c
> +++ b/src/bin/lttng/commands/enable_channels.c
> @@ -84,15 +84,15 @@
> fprintf(ofp, "\n");
> fprintf(ofp, " -h, --help Show this help\n");
> fprintf(ofp, " --list-options Simple listing of options\n");
> - fprintf(ofp, " -s, --session Apply on session name\n");
> - fprintf(ofp, " -k, --kernel Apply on the kernel tracer\n");
> + fprintf(ofp, " -s, --session NAME Apply to session name\n");
> + fprintf(ofp, " -k, --kernel Apply to the kernel tracer\n");
> #if 0
> - fprintf(ofp, " -u, --userspace [CMD] Apply for the user-space tracer\n");
> + fprintf(ofp, " -u, --userspace [CMD] Apply to the user-space tracer\n");
> fprintf(ofp, " If no CMD, the domain used is UST global\n");
> - fprintf(ofp, " or else the domain is UST EXEC_NAME\n");
> + fprintf(ofp, " otherwise the domain is UST EXEC_NAME\n");
> fprintf(ofp, " -p, --pid PID If -u, apply to specific PID (domain: UST PID)\n");
> #else
> - fprintf(ofp, " -u, --userspace Apply for the user-space tracer\n");
> + fprintf(ofp, " -u, --userspace Apply to the user-space tracer\n");
> #endif
> fprintf(ofp, "\n");
> fprintf(ofp, "Channel options:\n");
> @@ -100,17 +100,17 @@
> DEFAULT_CHANNEL_OVERWRITE ? "" : " (default)");
> fprintf(ofp, " --overwrite Flight recorder mode%s\n",
> DEFAULT_CHANNEL_OVERWRITE ? " (default)" : "");
> - fprintf(ofp, " --subbuf-size Subbuffer size in bytes\n");
> + fprintf(ofp, " --subbuf-size SIZE Subbuffer size in bytes\n");
> fprintf(ofp, " (default: %u, kernel default: %u)\n",
> DEFAULT_CHANNEL_SUBBUF_SIZE,
> DEFAULT_KERNEL_CHANNEL_SUBBUF_SIZE);
> - fprintf(ofp, " --num-subbuf Number of subbufers\n");
> + fprintf(ofp, " --num-subbuf NUM Number of subbufers\n");
> fprintf(ofp, " (default: %u, kernel default: %u)\n",
> DEFAULT_CHANNEL_SUBBUF_NUM,
> DEFAULT_KERNEL_CHANNEL_SUBBUF_NUM);
> - fprintf(ofp, " --switch-timer Switch timer interval in usec (default: %u)\n",
> + fprintf(ofp, " --switch-timer USEC Switch timer interval in usec (default: %u)\n",
> DEFAULT_CHANNEL_SWITCH_TIMER);
> - fprintf(ofp, " --read-timer Read timer interval in usec (default: %u)\n",
> + fprintf(ofp, " --read-timer USEC Read timer interval in usec (default: %u)\n",
> DEFAULT_CHANNEL_READ_TIMER);
> fprintf(ofp, "\n");
> }
> @@ -149,12 +149,14 @@
> /*
> * Adding channel using the lttng API.
> */
> +/* TODO Rename as enable_channels() to follow pattern of disable_channels.c */
> static int enable_channel(char *session_name)
> {
> int ret = CMD_SUCCESS;
> char *channel_name;
> struct lttng_domain dom;
>
> + /* TODO Enforce requirement of exactly one domain in cmd_...() */
> /* Create lttng domain */
> if (opt_kernel) {
> dom.type = LTTNG_DOMAIN_KERNEL;
> @@ -168,6 +170,7 @@
>
> set_default_attr(&dom);
>
> + /* TODO Document and enforce return values (e.g. -1 should be CMD_FATAL) */
> handle = lttng_create_handle(session_name, &dom);
> if (handle == NULL) {
> ret = -1;
> @@ -176,8 +179,11 @@
>
> /* Strip channel list (format: chan1,chan2,...) */
> channel_name = strtok(opt_channels, ",");
> + /* TODO Issue CMD_WARNING if some lttng_enable_channel() calls fail */
> + /* TODO or if opt_channels is effectively empty */
> while (channel_name != NULL) {
> /* Copy channel name and normalize it */
> + /* TODO Use LTTNG_SYMBOL_NAME_LEN instead of NAME_MAX */
> strncpy(chan.name, channel_name, NAME_MAX);
> chan.name[NAME_MAX - 1] = '\0';
>
> @@ -185,6 +191,7 @@
>
> ret = lttng_enable_channel(handle, &chan);
> if (ret < 0) {
> + /* TODO Issue ERR() here */
I've fixed it also in previous commit. :)
Thanks for all this.
David
> goto error;
> } else {
> MSG("%s channel %s enabled for session %s",
> @@ -295,6 +302,7 @@
> ret = enable_channel(session_name);
>
> end:
> + /* TODO Fix memory leak of session_name when opt_session_name == NULL */
> poptFreeContext(pc);
> return ret;
> }
> diff --git a/src/bin/lttng/commands/enable_events.c b/src/bin/lttng/commands/enable_events.c
> index f8bbc93..ad12fd2 100644
> --- a/src/bin/lttng/commands/enable_events.c
> +++ b/src/bin/lttng/commands/enable_events.c
> @@ -98,17 +98,17 @@
> fprintf(ofp, "\n");
> fprintf(ofp, " -h, --help Show this help\n");
> fprintf(ofp, " --list-options Simple listing of options\n");
> - fprintf(ofp, " -s, --session Apply on session name\n");
> - fprintf(ofp, " -c, --channel Apply on this channel\n");
> + fprintf(ofp, " -s, --session NAME Apply to session name\n");
> + fprintf(ofp, " -c, --channel NAME Apply to this channel\n");
> fprintf(ofp, " -a, --all Enable all tracepoints\n");
> - fprintf(ofp, " -k, --kernel Apply for the kernel tracer\n");
> + fprintf(ofp, " -k, --kernel Apply to the kernel tracer\n");
> #if 0
> - fprintf(ofp, " -u, --userspace [CMD] Apply for the user-space tracer\n");
> + fprintf(ofp, " -u, --userspace [CMD] Apply to the user-space tracer\n");
> fprintf(ofp, " If no CMD, the domain used is UST global\n");
> - fprintf(ofp, " or else the domain is UST EXEC_NAME\n");
> + fprintf(ofp, " otherwise the domain is UST EXEC_NAME\n");
> fprintf(ofp, " -p, --pid PID If -u, apply to specific PID (domain: UST PID)\n");
> #else
> - fprintf(ofp, " -u, --userspace Apply for the user-space tracer\n");
> + fprintf(ofp, " -u, --userspace Apply to the user-space tracer\n");
> #endif
> fprintf(ofp, "\n");
> fprintf(ofp, "Event options:\n");
> @@ -138,6 +138,8 @@
> /*
> * Parse probe options.
> */
> +/* TODO Document and enforce return values */
> +/* TODO On success, returns 0; on error, returns -1 */
> static int parse_probe_opts(struct lttng_event *ev, char *opt)
> {
> int ret;
> @@ -222,6 +224,7 @@
> channel_name = opt_channel_name;
> }
>
> + /* TODO Enforce requirement of exactly one domain in cmd_...() */
> if (opt_kernel && opt_userspace) {
> ERR("Can't use -k/--kernel and -u/--userspace together");
> ret = CMD_FATAL;
> @@ -239,6 +242,7 @@
> goto error;
> }
>
> + /* TODO Document and enforce return values (e.g. -1 should be CMD_FATAL) */
> handle = lttng_create_handle(session_name, &dom);
> if (handle == NULL) {
> ret = -1;
> @@ -272,6 +276,7 @@
> channel_name);
> }
> break;
> + /* TODO What of LTTNG_EVENT_SYSCALL in --userspace mode? */
> case LTTNG_EVENT_ALL:
> MSG("All %s events are enabled in channel %s",
> opt_kernel ? "kernel" : "UST", channel_name);
> @@ -281,6 +286,7 @@
> * We should not be here since lttng_enable_event should have
> * failed on the event type.
> */
> + /* TODO ret = CMD_FATAL; */
> goto error;
> }
> goto end;
> @@ -288,6 +294,8 @@
>
> /* Strip event list */
> event_name = strtok(opt_event_list, ",");
> + /* TODO Issue CMD_WARNING if some lttng_enable_event() calls fail */
> + /* TODO or if opt_event_list is effectively empty */
> while (event_name != NULL) {
> /* Copy name and type of the event */
> strncpy(ev.name, event_name, LTTNG_SYMBOL_NAME_LEN);
> diff --git a/src/bin/lttng/commands/list.c b/src/bin/lttng/commands/list.c
> index d09ee57..68e7ee4 100644
> --- a/src/bin/lttng/commands/list.c
> +++ b/src/bin/lttng/commands/list.c
> @@ -70,22 +70,22 @@
> */
> static void usage(FILE *ofp)
> {
> - fprintf(ofp, "usage: lttng list [SESSION [<OPTIONS>]]\n");
> + fprintf(ofp, "usage: lttng list [OPTIONS] [SESSION [SESSION_OPTIONS]]\n");
> fprintf(ofp, "\n");
> fprintf(ofp, "With no arguments, list available tracing session(s)\n");
> fprintf(ofp, "\n");
> - fprintf(ofp, "With -k alone, list available kernel events\n");
> - fprintf(ofp, "With -u alone, list available userspace events\n");
> + fprintf(ofp, "Without a session, -k lists available kernel events\n");
> + fprintf(ofp, "Without a session, -u lists available userspace events\n");
> fprintf(ofp, "\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, " -k, --kernel Select kernel domain\n");
> fprintf(ofp, " -u, --userspace Select user-space domain.\n");
> #if 0
> fprintf(ofp, " -p, --pid PID List user-space events by PID\n");
> #endif
> fprintf(ofp, "\n");
> - fprintf(ofp, "Options:\n");
> + fprintf(ofp, "Session Options:\n");
> fprintf(ofp, " -c, --channel NAME List details of a channel\n");
> fprintf(ofp, " -d, --domain List available domain(s)\n");
> fprintf(ofp, "\n");
> ------------------------------
>
> 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)
iQEcBAEBAgAGBQJPKcyfAAoJEELoaioR9I02JgAIAKGI/TYHipPqpgCIRxfwCAT/
1ulJduXeeNPZS2PB99+a7vcRKksTrgq0Z4GBNugLAQ9ECWTODqWHm3jZd0rLLi6E
r2sqFFbt5wQAOIIvW8Y467MvSUV4jmIQX/3jMuvJ7k1jcoVYoB9qnHTTk4rod+qI
W/a4yYQrfE7BbZSkDUFufQHvITPkAxnP2dohcClNwxlGXPVUrq0mi2ZLj/WAk3tc
kImuD2hyPILCTHYl5y8vJuVsjJpf/RNLXAHYuQJKFffoynplpC/So0QKiLEAXwLn
83lrDPzOisJBGX18IioFteduLfsJKFZIlBAgwNUF5hriW4r+qlps7IxsYihyyp0=
=x3Rx
-----END PGP SIGNATURE-----
More information about the lttng-dev
mailing list