From ae8564917fa3cb3497ec17951d8ac0ee28de9c81 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 1 Feb 2012 18:09:11 -0500 Subject: [PATCH] Improve error handling of lttng cli Most of the commands were not returning a valid error code to the user. Add "UST invalid context" lttcomm code and enforce "-t TYPE" for add-context command. Change error message for enable/disable event/channel in order to make it standard for those commands and more meaningful to the user. Signed-off-by: David Goulet --- src/bin/lttng-sessiond/context.c | 11 ++-- src/bin/lttng-sessiond/main.c | 1 - src/bin/lttng-sessiond/session.c | 2 +- src/bin/lttng/commands/add_context.c | 7 +++ src/bin/lttng/commands/calibrate.c | 1 - src/bin/lttng/commands/create.c | 2 +- src/bin/lttng/commands/destroy.c | 1 + src/bin/lttng/commands/disable_channels.c | 15 ++++-- src/bin/lttng/commands/disable_events.c | 66 +++++++++-------------- src/bin/lttng/commands/enable_channels.c | 12 ++++- src/bin/lttng/commands/enable_events.c | 38 ++++++------- src/bin/lttng/commands/list.c | 16 +++--- src/bin/lttng/commands/start.c | 5 +- src/bin/lttng/commands/stop.c | 5 +- src/bin/lttng/lttng.c | 53 +++++++++--------- src/common/sessiond-comm/sessiond-comm.c | 3 +- src/common/sessiond-comm/sessiond-comm.h | 3 +- 17 files changed, 131 insertions(+), 110 deletions(-) diff --git a/src/bin/lttng-sessiond/context.c b/src/bin/lttng-sessiond/context.c index 7eaeb6665..9727258bf 100644 --- a/src/bin/lttng-sessiond/context.c +++ b/src/bin/lttng-sessiond/context.c @@ -337,7 +337,7 @@ int context_ust_add(struct ltt_ust_session *usess, int domain, struct lttng_event_context *ctx, char *event_name, char *channel_name) { - int ret = LTTCOMM_OK, have_event = 0; + int ret = LTTCOMM_OK, have_event = 0, no_chan = 1; struct lttng_ht_iter iter; struct lttng_ht *chan_ht; struct ltt_ust_channel *uchan = NULL; @@ -409,9 +409,10 @@ int context_ust_add(struct ltt_ust_session *usess, int domain, } else if (!uchan && !have_event) { /* Add ctx all events, all channels */ /* For all channels */ cds_lfht_for_each_entry(chan_ht->ht, &iter.iter, uchan, node.node) { + no_chan = 0; ret = add_uctx_to_channel(usess, domain, uchan, ctx); if (ret < 0) { - ERR("Context added to channel %s failed", uchan->name); + ERR("Context failed for channel %s", uchan->name); continue; } } @@ -426,7 +427,7 @@ end: ret = LTTCOMM_FATAL; break; case -EINVAL: - ret = LTTCOMM_UST_CONTEXT_FAIL; + ret = LTTCOMM_UST_CONTEXT_INVAL; break; case -ENOSYS: ret = LTTCOMM_UNKNOWN_DOMAIN; @@ -436,6 +437,10 @@ end: break; } + if (no_chan) { + ret = LTTCOMM_UST_CHAN_NOT_FOUND; + } + error: return ret; } diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c index 2aa93e63e..e990c3484 100644 --- a/src/bin/lttng-sessiond/main.c +++ b/src/bin/lttng-sessiond/main.c @@ -3302,7 +3302,6 @@ static int process_client_msg(struct command_ctx *cmd_ctx) ret = cmd_disable_event(cmd_ctx->session, cmd_ctx->lsm->domain.type, cmd_ctx->lsm->u.disable.channel_name, cmd_ctx->lsm->u.disable.name); - ret = LTTCOMM_OK; break; } case LTTNG_DISABLE_ALL_EVENT: diff --git a/src/bin/lttng-sessiond/session.c b/src/bin/lttng-sessiond/session.c index 97fc94fe5..e890d07ba 100644 --- a/src/bin/lttng-sessiond/session.c +++ b/src/bin/lttng-sessiond/session.c @@ -221,7 +221,7 @@ int session_create(char *name, char *path, uid_t uid, gid_t gid) if (ret < 0) { if (ret != -EEXIST) { ERR("Trace directory creation error"); - ret = LTTCOMM_CREATE_FAIL; + ret = LTTCOMM_CREATE_DIR_FAIL; goto error; } } diff --git a/src/bin/lttng/commands/add_context.c b/src/bin/lttng/commands/add_context.c index a1d776d6c..ed4f87fb4 100644 --- a/src/bin/lttng/commands/add_context.c +++ b/src/bin/lttng/commands/add_context.c @@ -512,6 +512,13 @@ int cmd_add_context(int argc, const char **argv) } } + if (!opt_type) { + ERR("Missing mandatory -t TYPE"); + usage(stderr); + ret = CMD_ERROR; + goto end; + } + if (!opt_session_name) { session_name = get_session_name(); if (session_name == NULL) { diff --git a/src/bin/lttng/commands/calibrate.c b/src/bin/lttng/commands/calibrate.c index 547b349e4..dc95d2469 100644 --- a/src/bin/lttng/commands/calibrate.c +++ b/src/bin/lttng/commands/calibrate.c @@ -156,7 +156,6 @@ static int calibrate_lttng(void) calibrate.type = LTTNG_CALIBRATE_FUNCTION; ret = lttng_calibrate(handle, &calibrate); if (ret < 0) { - ret = CMD_ERROR; goto error; } MSG("%s calibration done", opt_kernel ? "Kernel" : "UST"); diff --git a/src/bin/lttng/commands/create.c b/src/bin/lttng/commands/create.c index 7ed663259..f60fe619e 100644 --- a/src/bin/lttng/commands/create.c +++ b/src/bin/lttng/commands/create.c @@ -114,7 +114,7 @@ static int create_session() ret = lttng_create_session(session_name, traces_path); if (ret < 0) { - ret = CMD_ERROR; + /* Don't set ret so lttng can interpret the sessiond error. */ goto error; } diff --git a/src/bin/lttng/commands/destroy.c b/src/bin/lttng/commands/destroy.c index b0262ea2f..dbd91c215 100644 --- a/src/bin/lttng/commands/destroy.c +++ b/src/bin/lttng/commands/destroy.c @@ -77,6 +77,7 @@ static int destroy_session() ret = lttng_destroy_session(session_name); if (ret < 0) { + /* Don't set ret so lttng can interpret the sessiond error. */ goto free_name; } diff --git a/src/bin/lttng/commands/disable_channels.c b/src/bin/lttng/commands/disable_channels.c index 05ee0e6ef..5fb90c5d1 100644 --- a/src/bin/lttng/commands/disable_channels.c +++ b/src/bin/lttng/commands/disable_channels.c @@ -88,7 +88,7 @@ static void usage(FILE *ofp) */ static int disable_channels(char *session_name) { - int ret = CMD_SUCCESS; + int ret = CMD_SUCCESS, warn = 0; char *channel_name; struct lttng_domain dom; @@ -116,18 +116,25 @@ static int disable_channels(char *session_name) ret = lttng_disable_channel(handle, channel_name); if (ret < 0) { - goto error; + ERR("Channel %s: %s (session %s)", channel_name, + lttng_strerror(ret), session_name); + warn = 1; } else { MSG("%s channel %s disabled for session %s", - opt_kernel ? "Kernel" : "UST", channel_name, - session_name); + opt_kernel ? "Kernel" : "UST", channel_name, session_name); } /* Next channel */ channel_name = strtok(NULL, ","); } + ret = CMD_SUCCESS; + error: + if (warn) { + ret = CMD_WARNING; + } + lttng_destroy_handle(handle); return ret; diff --git a/src/bin/lttng/commands/disable_events.c b/src/bin/lttng/commands/disable_events.c index 4ddcf5431..1e5c40a5f 100644 --- a/src/bin/lttng/commands/disable_events.c +++ b/src/bin/lttng/commands/disable_events.c @@ -96,26 +96,10 @@ static void usage(FILE *ofp) */ static int disable_events(char *session_name) { - int err, ret = CMD_SUCCESS; + int err, ret = CMD_SUCCESS, warn = 0; char *event_name, *channel_name = NULL; struct lttng_domain dom; - if (opt_channel_name == NULL) { - err = asprintf(&channel_name, DEFAULT_CHANNEL_NAME); - if (err < 0) { - ret = CMD_FATAL; - goto error; - } - } else { - channel_name = opt_channel_name; - } - - if (opt_kernel && opt_userspace) { - ERR("Can't use -k/--kernel and -u/--userspace together"); - ret = CMD_FATAL; - goto error; - } - /* Create lttng domain */ if (opt_kernel) { dom.type = LTTNG_DOMAIN_KERNEL; @@ -127,6 +111,17 @@ static int disable_events(char *session_name) goto error; } + /* Get channel name */ + if (opt_channel_name == NULL) { + err = asprintf(&channel_name, DEFAULT_CHANNEL_NAME); + if (err < 0) { + ret = CMD_FATAL; + goto error; + } + } else { + channel_name = opt_channel_name; + } + handle = lttng_create_handle(session_name, &dom); if (handle == NULL) { ret = -1; @@ -136,6 +131,7 @@ static int disable_events(char *session_name) if (opt_disable_all) { ret = lttng_disable_event(handle, NULL, channel_name); if (ret < 0) { + /* Don't set ret so lttng can interpret the sessiond error. */ goto error; } @@ -147,42 +143,30 @@ static int disable_events(char *session_name) /* Strip event list */ event_name = strtok(opt_event_list, ","); while (event_name != NULL) { - /* Kernel tracer action */ - if (opt_kernel) { - DBG("Disabling kernel event %s in channel %s", - event_name, channel_name); - } else if (opt_userspace) { /* User-space tracer action */ -#if 0 - if (opt_cmd_name != NULL || opt_pid) { - MSG("Only supporting tracing all UST processes (-u) for now."); - ret = CMD_UNDEFINED; - goto error; - } -#endif - DBG("Disabling UST event %s in channel %s", - event_name, channel_name); - } else { - ERR("Please specify a tracer (-k/--kernel or -u/--userspace)"); - goto error; - } + DBG("Disabling event %s", event_name); ret = lttng_disable_event(handle, event_name, channel_name); if (ret < 0) { - MSG("Unable to disable %s event %s in channel %s", - opt_kernel ? "kernel" : "UST", event_name, - channel_name); + ERR("Event %s: %s (channel %s, session %s)", event_name, + lttng_strerror(ret), channel_name, session_name); + warn = 1; } else { - MSG("%s event %s disabled in channel %s", - opt_kernel ? "kernel" : "UST", event_name, - channel_name); + MSG("%s event %s disabled in channel %s for session %s", + opt_kernel ? "kernel" : "UST", event_name, channel_name, + session_name); } /* Next event */ event_name = strtok(NULL, ","); } + ret = CMD_SUCCESS; + end: error: + if (warn) { + ret = CMD_WARNING; + } if (opt_channel_name == NULL) { free(channel_name); } diff --git a/src/bin/lttng/commands/enable_channels.c b/src/bin/lttng/commands/enable_channels.c index 19b6b260b..942dbf00e 100644 --- a/src/bin/lttng/commands/enable_channels.c +++ b/src/bin/lttng/commands/enable_channels.c @@ -151,7 +151,7 @@ static void set_default_attr(struct lttng_domain *dom) */ static int enable_channel(char *session_name) { - int ret = CMD_SUCCESS; + int ret = CMD_SUCCESS, warn = 0; char *channel_name; struct lttng_domain dom; @@ -185,7 +185,9 @@ static int enable_channel(char *session_name) ret = lttng_enable_channel(handle, &chan); if (ret < 0) { - goto error; + ERR("Channel %s: %s (session %s)", channel_name, + lttng_strerror(ret), session_name); + warn = 1; } else { MSG("%s channel %s enabled for session %s", opt_kernel ? "Kernel" : "UST", channel_name, @@ -196,7 +198,13 @@ static int enable_channel(char *session_name) channel_name = strtok(NULL, ","); } + ret = CMD_SUCCESS; + error: + if (warn) { + ret = CMD_WARNING; + } + lttng_destroy_handle(handle); return ret; diff --git a/src/bin/lttng/commands/enable_events.c b/src/bin/lttng/commands/enable_events.c index 49bae56b2..5beb5a2b2 100644 --- a/src/bin/lttng/commands/enable_events.c +++ b/src/bin/lttng/commands/enable_events.c @@ -214,27 +214,11 @@ end: */ static int enable_events(char *session_name) { - int err, ret = CMD_SUCCESS; + int err, ret = CMD_SUCCESS, warn = 0; char *event_name, *channel_name = NULL; struct lttng_event ev; struct lttng_domain dom; - if (opt_channel_name == NULL) { - err = asprintf(&channel_name, DEFAULT_CHANNEL_NAME); - if (err < 0) { - ret = CMD_FATAL; - goto error; - } - } else { - channel_name = opt_channel_name; - } - - if (opt_kernel && opt_userspace) { - ERR("Can't use -k/--kernel and -u/--userspace together"); - ret = CMD_FATAL; - goto error; - } - /* Create lttng domain */ if (opt_kernel) { dom.type = LTTNG_DOMAIN_KERNEL; @@ -246,6 +230,16 @@ static int enable_events(char *session_name) goto error; } + if (opt_channel_name == NULL) { + err = asprintf(&channel_name, DEFAULT_CHANNEL_NAME); + if (err < 0) { + ret = CMD_FATAL; + goto error; + } + } else { + channel_name = opt_channel_name; + } + handle = lttng_create_handle(session_name, &dom); if (handle == NULL) { ret = -1; @@ -254,7 +248,6 @@ static int enable_events(char *session_name) if (opt_enable_all) { /* Default setup for enable all */ - if (opt_kernel) { ev.type = opt_event_type; ev.name[0] = '\0'; @@ -393,7 +386,11 @@ static int enable_events(char *session_name) } ret = lttng_enable_event(handle, &ev, channel_name); - if (ret == 0) { + if (ret < 0) { + ERR("Event %s: %s (channel %s, session %s)", event_name, + lttng_strerror(ret), channel_name, session_name); + warn = 1; + } else { MSG("%s event %s created in channel %s", opt_kernel ? "kernel": "UST", event_name, channel_name); } @@ -404,6 +401,9 @@ static int enable_events(char *session_name) end: error: + if (warn) { + ret = CMD_WARNING; + } if (opt_channel_name == NULL) { free(channel_name); } diff --git a/src/bin/lttng/commands/list.c b/src/bin/lttng/commands/list.c index d84fd2ba9..4def4e7a2 100644 --- a/src/bin/lttng/commands/list.c +++ b/src/bin/lttng/commands/list.c @@ -389,10 +389,10 @@ static int list_channels(const char *channel_name) count = lttng_list_channels(handle, &channels); if (count < 0) { ret = count; - goto error; + goto error_channels; } else if (count == 0) { - MSG("No channel found"); - goto end; + ERR("Channel %s not found", channel_name); + goto error; } if (channel_name == NULL) { @@ -421,14 +421,16 @@ static int list_channels(const char *channel_name) } if (!chan_found && channel_name != NULL) { - MSG("Channel %s not found", channel_name); + ERR("Channel %s not found", channel_name); + goto error; } -end: - free(channels); ret = CMD_SUCCESS; error: + free(channels); + +error_channels: return ret; } @@ -475,7 +477,7 @@ static int list_sessions(const char *session_name) free(sessions); if (!session_found && session_name != NULL) { - MSG("Session %s not found", session_name); + ERR("Session %s not found", session_name); } if (session_name == NULL) { diff --git a/src/bin/lttng/commands/start.c b/src/bin/lttng/commands/start.c index 1db0a31c5..af93665d0 100644 --- a/src/bin/lttng/commands/start.c +++ b/src/bin/lttng/commands/start.c @@ -63,7 +63,7 @@ static void usage(FILE *ofp) */ static int start_tracing(void) { - int ret = CMD_SUCCESS; + int ret; char *session_name; if (opt_session_name == NULL) { @@ -80,9 +80,12 @@ static int start_tracing(void) ret = lttng_start_tracing(session_name); if (ret < 0) { + /* Don't set ret so lttng can interpret the sessiond error. */ goto free_name; } + ret = CMD_SUCCESS; + MSG("Tracing started for session %s", session_name); free_name: diff --git a/src/bin/lttng/commands/stop.c b/src/bin/lttng/commands/stop.c index 689fe1b04..f2ab45308 100644 --- a/src/bin/lttng/commands/stop.c +++ b/src/bin/lttng/commands/stop.c @@ -61,7 +61,7 @@ static void usage(FILE *ofp) */ static int stop_tracing(void) { - int ret = CMD_SUCCESS; + int ret; char *session_name; if (opt_session_name == NULL) { @@ -76,9 +76,12 @@ static int stop_tracing(void) ret = lttng_stop_tracing(session_name); if (ret < 0) { + /* Don't set ret so lttng can interpret the sessiond error. */ goto free_name; } + ret = CMD_SUCCESS; + MSG("Tracing stopped for session %s", session_name); free_name: diff --git a/src/bin/lttng/lttng.c b/src/bin/lttng/lttng.c index 71bc1bf77..438320d97 100644 --- a/src/bin/lttng/lttng.c +++ b/src/bin/lttng/lttng.c @@ -264,20 +264,6 @@ static int handle_command(int argc, char **argv) /* Find command */ if (strcmp(argv[0], cmd->name) == 0) { ret = cmd->func(argc, (const char**) argv); - switch (ret) { - case CMD_WARNING: - WARN("Some command(s) went wrong"); - break; - case CMD_ERROR: - ERR("Command error"); - break; - case CMD_UNDEFINED: - ERR("Undefined command"); - break; - case CMD_FATAL: - ERR("Fatal error"); - break; - } goto end; } i++; @@ -439,6 +425,7 @@ static int parse_args(int argc, char **argv) switch (opt) { case 'h': usage(stdout); + ret = 0; goto end; case 'v': opt_verbose += 1; @@ -465,6 +452,7 @@ static int parse_args(int argc, char **argv) goto end; default: usage(stderr); + ret = 1; goto error; } } @@ -491,20 +479,33 @@ static int parse_args(int argc, char **argv) * options. */ ret = handle_command(argc - optind, argv + optind); - if (ret < 0) { - if (ret == -1) { - usage(stderr); - } else { - ERR("%s", lttng_strerror(ret)); - } - goto error; + switch (ret) { + case CMD_WARNING: + WARN("Some command(s) went wrong"); + break; + case CMD_ERROR: + ERR("Command error"); + break; + case CMD_UNDEFINED: + ERR("Undefined command"); + break; + case CMD_FATAL: + ERR("Fatal error"); + break; + case -1: + usage(stderr); + ret = 1; + break; + case 0: + break; + default: + ERR("%s", lttng_strerror(ret)); + break; } end: - return 0; - error: - return -1; + return ret; } @@ -529,8 +530,8 @@ int main(int argc, char *argv[]) } ret = parse_args(argc, argv); - if (ret < 0) { - clean_exit(EXIT_FAILURE); + if (ret != 0) { + clean_exit(ret); } return 0; diff --git a/src/common/sessiond-comm/sessiond-comm.c b/src/common/sessiond-comm/sessiond-comm.c index 591e9629a..24757f702 100644 --- a/src/common/sessiond-comm/sessiond-comm.c +++ b/src/common/sessiond-comm/sessiond-comm.c @@ -47,7 +47,7 @@ static const char *lttcomm_readable_code[] = { [ LTTCOMM_ERR_INDEX(LTTCOMM_SESS_NOT_FOUND) ] = "Session name not found", [ LTTCOMM_ERR_INDEX(LTTCOMM_NO_TRACE) ] = "No trace found", [ LTTCOMM_ERR_INDEX(LTTCOMM_FATAL) ] = "Fatal error of the session daemon", - [ LTTCOMM_ERR_INDEX(LTTCOMM_CREATE_FAIL) ] = "Create trace failed", + [ LTTCOMM_ERR_INDEX(LTTCOMM_CREATE_DIR_FAIL) ] = "Create directory failed", [ LTTCOMM_ERR_INDEX(LTTCOMM_START_FAIL) ] = "Start trace failed", [ LTTCOMM_ERR_INDEX(LTTCOMM_STOP_FAIL) ] = "Stop trace failed", [ LTTCOMM_ERR_INDEX(LTTCOMM_NO_TRACEABLE) ] = "App is not traceable", @@ -100,6 +100,7 @@ static const char *lttcomm_readable_code[] = { [ LTTCOMM_ERR_INDEX(LTTCOMM_UST_EVENT_EXIST) ] = "UST event already exist", [ LTTCOMM_ERR_INDEX(LTTCOMM_UST_EVENT_NOT_FOUND)] = "UST event not found", [ LTTCOMM_ERR_INDEX(LTTCOMM_UST_CONTEXT_EXIST)] = "UST context already exist", + [ LTTCOMM_ERR_INDEX(LTTCOMM_UST_CONTEXT_INVAL)] = "UST invalid context", [ LTTCOMM_ERR_INDEX(CONSUMERD_COMMAND_SOCK_READY) ] = "consumerd command socket ready", [ LTTCOMM_ERR_INDEX(CONSUMERD_SUCCESS_RECV_FD) ] = "consumerd success on receiving fds", [ LTTCOMM_ERR_INDEX(CONSUMERD_ERROR_RECV_FD) ] = "consumerd error on receiving fds", diff --git a/src/common/sessiond-comm/sessiond-comm.h b/src/common/sessiond-comm/sessiond-comm.h index d8d27063d..31b7fe466 100644 --- a/src/common/sessiond-comm/sessiond-comm.h +++ b/src/common/sessiond-comm/sessiond-comm.h @@ -75,7 +75,7 @@ enum lttcomm_return_code { LTTCOMM_UNKNOWN_DOMAIN, /* Tracing domain not known */ LTTCOMM_ALLOC_FAIL, /* Trace allocation fail */ LTTCOMM_NO_SESSION, /* No session found */ - LTTCOMM_CREATE_FAIL, /* Create trace fail */ + LTTCOMM_CREATE_DIR_FAIL, /* Create directory fail */ LTTCOMM_SESSION_FAIL, /* Create session fail */ LTTCOMM_START_FAIL, /* Start tracing fail */ LTTCOMM_STOP_FAIL, /* Stop tracing fail */ @@ -135,6 +135,7 @@ enum lttcomm_return_code { LTTCOMM_UST_EVENT_EXIST, /* UST event exist */ LTTCOMM_UST_EVENT_NOT_FOUND, /* UST event not found */ LTTCOMM_UST_CONTEXT_EXIST, /* UST context exist */ + LTTCOMM_UST_CONTEXT_INVAL, /* UST context invalid */ CONSUMERD_COMMAND_SOCK_READY, /* when consumerd command socket ready */ CONSUMERD_SUCCESS_RECV_FD, /* success on receiving fds */ -- 2.34.1