-----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 <[email protected]> > 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 <[email protected]> > > 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 > [email protected] > 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----- _______________________________________________ lttng-dev mailing list [email protected] http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
