>From 288de853ac784b52a6da3dff3918e88ac80c599e Tue, 31 Jan 2012 17:27:14 -0500
From: Daniel U. Thibault <[email protected]>
Date: Tue, 31 Jan 2012 17:26:57 -0500
Subject: [PATCH] enable_events and disable_events : Improve usage() printout, 
document and enforce return values, send --help to stdout, setup TODOs

Signed-off-by: Daniel U. Thibault <[email protected]>

diff --git a/src/bin/lttng/commands/disable_events.c 
b/src/bin/lttng/commands/disable_events.c
index 4d21f68..23f4858 100644
--- a/src/bin/lttng/commands/disable_events.c
+++ b/src/bin/lttng/commands/disable_events.c
@@ -74,25 +74,26 @@
        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, "  -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, "                           (--kernel preempts 
--userspace)\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");
+       fprintf(ofp, "                           (--kernel preempts 
--userspace)\n");
 #endif
        fprintf(ofp, "\n");
 }
 
 /*
- *  disable_events
- *
- *  Disabling event using the lttng API.
+ *  Disabling event(s) using the lttng API.
+ *  Returns a CMD_* result value.
  */
 static int disable_events(char *session_name)
 {
@@ -110,6 +111,7 @@
                channel_name = opt_channel_name;
        }
 
+       /* TODO: Either do this test everywhere or don't do it anywhere */
        if (opt_kernel && opt_userspace) {
                ERR("Can't use -k/--kernel and -u/--userspace together");
                ret = CMD_FATAL;
@@ -129,13 +131,14 @@
 
        handle = lttng_create_handle(session_name, &dom);
        if (handle == NULL) {
-               ret = -1;
+               ret = CMD_ERROR;
                goto error;
        }
 
        if (opt_disable_all) {
                ret = lttng_disable_event(handle, NULL, channel_name);
                if (ret < 0) {
+                       ret = CMD_ERROR;
                        goto error;
                }
 
@@ -146,6 +149,7 @@
 
        /* Strip event list */
        event_name = strtok(opt_event_list, ",");
+       /* TODO: Issue CMD_WARNING if one or more events fail, or if the event 
list is empty */
        while (event_name != NULL) {
                /* Kernel tracer action */
                if (opt_kernel) {
@@ -163,11 +167,13 @@
                                        event_name, channel_name);
                } else {
                        ERR("Please specify a tracer (-k/--kernel or 
-u/--userspace)");
+                       ret = CMD_ERROR;
                        goto error;
                }
 
                ret = lttng_disable_event(handle, event_name, channel_name);
                if (ret < 0) {
+                       ret = CMD_WARNING;
                        MSG("Unable to disable %s event %s in channel %s",
                                        opt_kernel ? "kernel" : "UST", 
event_name,
                                        channel_name);
@@ -192,9 +198,9 @@
 }
 
 /*
- *  cmd_disable_events
- *
- *  Disable event to trace session
+ *  Disable event(s) of trace session
+ *  An empty event list or misnamed events will yield a warning.
+ *  Returns a CMD_* result value.
  */
 int cmd_disable_events(int argc, const char **argv)
 {
@@ -208,7 +214,7 @@
        while ((opt = poptGetNextOpt(pc)) != -1) {
                switch (opt) {
                case OPT_HELP:
-                       usage(stderr);
+                       usage(stdout);
                        ret = CMD_SUCCESS;
                        goto end;
                case OPT_USERSPACE:
@@ -229,14 +235,14 @@
        if (opt_event_list == NULL && opt_disable_all == 0) {
                ERR("Missing event name(s).\n");
                usage(stderr);
-               ret = CMD_SUCCESS;
+               ret = CMD_ERROR;
                goto end;
        }
 
        if (!opt_session_name) {
                session_name = get_session_name();
                if (session_name == NULL) {
-                       ret = -1;
+                       ret = CMD_ERROR;
                        goto end;
                }
        } else {
@@ -246,6 +252,7 @@
        ret = disable_events(session_name);
 
 end:
+       /* TODO: Free session_name when opt_session_name is NULL */
        poptFreeContext(pc);
        return ret;
 }
diff --git a/src/bin/lttng/commands/enable_events.c 
b/src/bin/lttng/commands/enable_events.c
index 01fae84..566ed61 100644
--- a/src/bin/lttng/commands/enable_events.c
+++ b/src/bin/lttng/commands/enable_events.c
@@ -98,17 +98,19 @@
        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, "                           (--kernel preempts 
--userspace)\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");
+       fprintf(ofp, "                           (--kernel preempts 
--userspace)\n");
 #endif
        fprintf(ofp, "\n");
        fprintf(ofp, "Event options:\n");
@@ -137,6 +139,7 @@
 
 /*
  * Parse probe options.
+ * On success, returns 0; on error, returns -1
  */
 static int parse_probe_opts(struct lttng_event *ev, char *opt)
 {
@@ -163,6 +166,7 @@
                ev->attr.probe.offset = strtoul(s_hex, NULL, 0);
                DBG("probe offset %" PRIu64, ev->attr.probe.offset);
                ev->attr.probe.addr = 0;
+               ret = 0; /* Success */
                goto end;
        }
 
@@ -176,6 +180,7 @@
                        ev->attr.probe.offset = 0;
                        DBG("probe offset %" PRIu64, ev->attr.probe.offset);
                        ev->attr.probe.addr = 0;
+                       ret = 0; /* Success */
                        goto end;
                }
        }
@@ -192,6 +197,7 @@
                DBG("probe addr %" PRIu64, ev->attr.probe.addr);
                ev->attr.probe.offset = 0;
                memset(ev->attr.probe.symbol_name, 0, LTTNG_SYMBOL_NAME_LEN);
+               ret = 0; /* Success */
                goto end;
        }
 
@@ -203,7 +209,8 @@
 }
 
 /*
- * Enabling event using the lttng API.
+ * Enabling event(s) using the lttng API.
+ * Returns a CMD_* result value.
  */
 static int enable_events(char *session_name)
 {
@@ -222,6 +229,7 @@
                channel_name = opt_channel_name;
        }
 
+       /* TODO: Either do this test everywhere or don't do it anywhere */
        if (opt_kernel && opt_userspace) {
                ERR("Can't use -k/--kernel and -u/--userspace together");
                ret = CMD_FATAL;
@@ -241,7 +249,7 @@
 
        handle = lttng_create_handle(session_name, &dom);
        if (handle == NULL) {
-               ret = -1;
+               ret = CMD_ERROR;
                goto error;
        }
 
@@ -258,8 +266,10 @@
 
                ret = lttng_enable_event(handle, &ev, channel_name);
                if (ret < 0) {
+                       ret = CMD_ERROR;
                        goto error;
                }
+               ret = CMD_SUCCESS;
 
                switch (opt_event_type) {
                case LTTNG_EVENT_TRACEPOINT:
@@ -271,6 +281,7 @@
                                MSG("All kernel system calls are enabled in 
channel %s",
                                                channel_name);
                        }
+                       /* TODO: What of LTTNG_EVENT_SYSCALL in --userspace 
mode? */
                        break;
                case LTTNG_EVENT_ALL:
                        MSG("All %s events are enabled in channel %s",
@@ -281,6 +292,7 @@
                         * We should not be here since lttng_enable_event 
should have
                         * failed on the event type.
                         */
+                       ret = CMD_FATAL;
                        goto error;
                }
                goto end;
@@ -288,6 +300,7 @@
 
        /* Strip event list */
        event_name = strtok(opt_event_list, ",");
+       /* TODO: Issue CMD_WARNING if one or more events fail, or if the event 
list is empty */
        while (event_name != NULL) {
                /* Copy name and type of the event */
                strncpy(ev.name, event_name, LTTNG_SYMBOL_NAME_LEN);
@@ -309,7 +322,7 @@
                                ret = parse_probe_opts(&ev, opt_probe);
                                if (ret < 0) {
                                        ERR("Unable to parse probe options");
-                                       ret = 0;
+                                       ret = CMD_ERROR;
                                        goto error;
                                }
                                break;
@@ -317,7 +330,7 @@
                                ret = parse_probe_opts(&ev, opt_function);
                                if (ret < 0) {
                                        ERR("Unable to parse function probe 
options");
-                                       ret = 0;
+                                       ret = CMD_ERROR;
                                        goto error;
                                }
                                break;
@@ -370,6 +383,7 @@
                        }
                } else {
                        ERR("Please specify a tracer (-k/--kernel or 
-u/--userspace)");
+                       ret = CMD_ERROR;
                        goto error;
                }
 
@@ -378,6 +392,7 @@
                        MSG("%s event %s created in channel %s",
                                        opt_kernel ? "kernel": "UST", 
event_name, channel_name);
                }
+               ret = CMD_SUCCESS;
 
                /* Next event */
                event_name = strtok(NULL, ",");
@@ -394,7 +409,8 @@
 }
 
 /*
- * Add event to trace session
+ * Add event(s) to trace session
+ * Returns a CMD_* result value.
  */
 int cmd_enable_events(int argc, const char **argv)
 {
@@ -411,7 +427,7 @@
        while ((opt = poptGetNextOpt(pc)) != -1) {
                switch (opt) {
                case OPT_HELP:
-                       usage(stderr);
+                       usage(stdout);
                        ret = CMD_SUCCESS;
                        goto end;
                case OPT_TRACEPOINT:
@@ -450,14 +466,14 @@
        if (opt_event_list == NULL && opt_enable_all == 0) {
                ERR("Missing event name(s).\n");
                usage(stderr);
-               ret = CMD_SUCCESS;
+               ret = CMD_ERROR;
                goto end;
        }
 
        if (!opt_session_name) {
                session_name = get_session_name();
                if (session_name == NULL) {
-                       ret = -1;
+                       ret = CMD_ERROR;
                        goto end;
                }
        } else {
------------------------------

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

Reply via email to