From 5e16da05d15eb413f28dea441e1bc49809cddc9b Mon Sep 17 00:00:00 2001 From: Mathieu Desnoyers Date: Fri, 13 May 2011 14:17:34 -0400 Subject: [PATCH] Comments and cleanup review Mathieu & David Signed-off-by: Mathieu Desnoyers Signed-off-by: David Goulet --- include/lttng/lttng.h | 1 + liblttsessiondcomm/liblttsessiondcomm.h | 7 +- ltt-sessiond/main.c | 237 +++++++++++++----------- ltt-sessiond/session.h | 5 + ltt-sessiond/trace.c | 3 + lttng/lttng.c | 39 ++-- lttng/options.c | 6 +- 7 files changed, 159 insertions(+), 139 deletions(-) diff --git a/include/lttng/lttng.h b/include/lttng/lttng.h index 3bb245239..957484f09 100644 --- a/include/lttng/lttng.h +++ b/include/lttng/lttng.h @@ -60,6 +60,7 @@ struct lttng_trace { enum lttng_trace_type type; }; +/* TODO: don't export these into system-installed headers. */ /* * LTTng DebugFS ABI structures. */ diff --git a/liblttsessiondcomm/liblttsessiondcomm.h b/liblttsessiondcomm/liblttsessiondcomm.h index 5cac0b798..adb9a8d84 100644 --- a/liblttsessiondcomm/liblttsessiondcomm.h +++ b/liblttsessiondcomm/liblttsessiondcomm.h @@ -24,6 +24,11 @@ #include #include +/* + * FIXME: 32, 64bit enums -> uint32_t uint64_t for data exchange. + * Same for pid_t. + */ + #define LTTNG_RUNDIR "/var/run/lttng" /* Default unix socket path */ @@ -145,7 +150,7 @@ struct lttcomm_session_msg { }; /* - * Data structure for the lttng client response. + * Data structure for the response from sessiond to the lttng client. * * This data structure is the control struct use in * the header of the transmission. NEVER put variable diff --git a/ltt-sessiond/main.c b/ltt-sessiond/main.c index f5431bc0a..21b05fc86 100644 --- a/ltt-sessiond/main.c +++ b/ltt-sessiond/main.c @@ -44,6 +44,13 @@ #include "trace.h" #include "traceable-app.h" +/* + * TODO: + * teardown: signal SIGTERM handler -> write into pipe. Threads waits + * with epoll on pipe and on other pipes/sockets for commands. Main + * simply waits on pthread join. + */ + /* Const values */ const char default_home_dir[] = DEFAULT_HOME_DIR; const char default_tracing_group[] = DEFAULT_TRACING_GROUP; @@ -136,6 +143,9 @@ static void *thread_manage_apps(void *data) int sock, ret; /* TODO: Something more elegant is needed but fine for now */ + /* FIXME: change all types to either uint8_t, uint32_t, uint64_t + * for 32-bit vs 64-bit compat processes. */ + /* replicate in ust with version number */ struct { int reg; /* 1:register, 0:unregister */ pid_t pid; @@ -144,14 +154,14 @@ static void *thread_manage_apps(void *data) DBG("[thread] Manage apps started"); - /* Notify all applications to register */ - notify_apps(default_global_apps_pipe); - ret = lttcomm_listen_unix_sock(apps_sock); if (ret < 0) { goto error; } + /* Notify all applications to register */ + notify_apps(default_global_apps_pipe); + while (1) { /* Blocking call, waiting for transmission */ sock = lttcomm_accept_unix_sock(apps_sock); @@ -403,15 +413,18 @@ static int process_client_msg(int sock, struct lttcomm_session_msg *lsm) copy_common_data(&llh, lsm); /* Check command that needs a session */ - if (lsm->cmd_type != LTTNG_CREATE_SESSION && - lsm->cmd_type != LTTNG_LIST_SESSIONS && - lsm->cmd_type != UST_LIST_APPS) - { + switch (lsm->cmd_type) { + case LTTNG_CREATE_SESSION: + case LTTNG_LIST_SESSIONS: + case UST_LIST_APPS: + break; + default: current_session = find_session_by_uuid(lsm->session_id); if (current_session == NULL) { ret = LTTCOMM_SELECT_SESS; goto end; } + break; } /* Default return code. @@ -433,133 +446,131 @@ static int process_client_msg(int sock, struct lttcomm_session_msg *lsm) /* Process by command type */ switch (lsm->cmd_type) { - case LTTNG_CREATE_SESSION: - { - ret = create_session(lsm->session_name, &llh.session_id); - if (ret < 0) { - if (ret == -1) { - ret = LTTCOMM_EXIST_SESS; - } else { - ret = LTTCOMM_FATAL; - } - goto end; - } - - buf_size = setup_data_buffer(&send_buf, 0, &llh); - if (buf_size < 0) { + case LTTNG_CREATE_SESSION: + { + ret = create_session(lsm->session_name, &llh.session_id); + if (ret < 0) { + if (ret == -1) { + ret = LTTCOMM_EXIST_SESS; + } else { ret = LTTCOMM_FATAL; - goto end; } - - break; + goto end; } - case LTTNG_DESTROY_SESSION: - { - ret = destroy_session(&lsm->session_id); - if (ret < 0) { - ret = LTTCOMM_NO_SESS; - } else { - ret = LTTCOMM_OK; - } - /* No auxiliary data so only send the llh struct. */ + buf_size = setup_data_buffer(&send_buf, 0, &llh); + if (buf_size < 0) { + ret = LTTCOMM_FATAL; goto end; } - case LTTNG_LIST_TRACES: - { - unsigned int trace_count = get_trace_count_per_session(current_session); - if (trace_count == 0) { - ret = LTTCOMM_NO_TRACE; - goto end; - } + break; + } + case LTTNG_DESTROY_SESSION: + { + ret = destroy_session(&lsm->session_id); + if (ret < 0) { + ret = LTTCOMM_NO_SESS; + } else { + ret = LTTCOMM_OK; + } - buf_size = setup_data_buffer(&send_buf, - sizeof(struct lttng_trace) * trace_count, &llh); - if (buf_size < 0) { - ret = LTTCOMM_FATAL; - goto end; - } + /* No auxiliary data so only send the llh struct. */ + goto end; + } + case LTTNG_LIST_TRACES: + { + unsigned int trace_count = get_trace_count_per_session(current_session); - get_traces_per_session(current_session, (struct lttng_trace *)(send_buf + header_size)); - break; + if (trace_count == 0) { + ret = LTTCOMM_NO_TRACE; + goto end; } - case UST_CREATE_TRACE: - { - ret = ust_create_trace(ust_sock, lsm->pid); - if (ret < 0) { - /* If -1 is returned from ust_create_trace, malloc - * failed so it's pretty much a fatal error. - */ - ret = LTTCOMM_FATAL; - goto end; - } - /* No auxiliary data so only send the llh struct. */ + buf_size = setup_data_buffer(&send_buf, + sizeof(struct lttng_trace) * trace_count, &llh); + if (buf_size < 0) { + ret = LTTCOMM_FATAL; goto end; } - case UST_LIST_APPS: - { - unsigned int app_count = get_app_count(); - /* Stop right now if no apps */ - if (app_count == 0) { - ret = LTTCOMM_NO_APPS; - goto end; - } - - /* Setup data buffer and details for transmission */ - buf_size = setup_data_buffer(&send_buf, - sizeof(pid_t) * app_count, &llh); - if (buf_size < 0) { - ret = LTTCOMM_FATAL; - goto end; - } - get_app_list_pids((pid_t *)(send_buf + header_size)); - - break; + get_traces_per_session(current_session, (struct lttng_trace *)(send_buf + header_size)); + break; + } + case UST_CREATE_TRACE: + { + ret = ust_create_trace(ust_sock, lsm->pid); + if (ret < 0) { + /* If -1 is returned from ust_create_trace, malloc + * failed so it's pretty much a fatal error. + */ + ret = LTTCOMM_FATAL; + goto end; } - case UST_START_TRACE: - { - ret = ust_start_trace(ust_sock, lsm->pid); - /* No auxiliary data so only send the llh struct. */ + /* No auxiliary data so only send the llh struct. */ + goto end; + } + case UST_LIST_APPS: + { + unsigned int app_count = get_app_count(); + /* Stop right now if no apps */ + if (app_count == 0) { + ret = LTTCOMM_NO_APPS; goto end; } - case UST_STOP_TRACE: - { - ret = ust_stop_trace(ust_sock, lsm->pid); - /* No auxiliary data so only send the llh struct. */ + /* Setup data buffer and details for transmission */ + buf_size = setup_data_buffer(&send_buf, + sizeof(pid_t) * app_count, &llh); + if (buf_size < 0) { + ret = LTTCOMM_FATAL; goto end; } - case LTTNG_LIST_SESSIONS: - { - unsigned int session_count = get_session_count(); - /* Stop right now if no session */ - if (session_count == 0) { - ret = LTTCOMM_NO_SESS; - goto end; - } - /* Setup data buffer and details for transmission */ - buf_size = setup_data_buffer(&send_buf, - (sizeof(struct lttng_session) * session_count), &llh); - if (buf_size < 0) { - ret = LTTCOMM_FATAL; - goto end; - } + get_app_list_pids((pid_t *)(send_buf + header_size)); + + break; + } + case UST_START_TRACE: + { + ret = ust_start_trace(ust_sock, lsm->pid); - get_lttng_session((struct lttng_session *)(send_buf + header_size)); + /* No auxiliary data so only send the llh struct. */ + goto end; + } + case UST_STOP_TRACE: + { + ret = ust_stop_trace(ust_sock, lsm->pid); - break; + /* No auxiliary data so only send the llh struct. */ + goto end; + } + case LTTNG_LIST_SESSIONS: + { + unsigned int session_count = get_session_count(); + /* Stop right now if no session */ + if (session_count == 0) { + ret = LTTCOMM_NO_SESS; + goto end; } - default: - { - /* Undefined command */ - ret = LTTCOMM_UND; + + /* Setup data buffer and details for transmission */ + buf_size = setup_data_buffer(&send_buf, + (sizeof(struct lttng_session) * session_count), &llh); + if (buf_size < 0) { + ret = LTTCOMM_FATAL; goto end; } + + get_lttng_session((struct lttng_session *)(send_buf + header_size)); + + break; + } + default: + /* Undefined command */ + ret = LTTCOMM_UND; + goto end; } ret = send_unix_sock(sock, send_buf, buf_size); @@ -769,7 +780,10 @@ static const char *get_home_dir(void) /* * set_permissions * - * Set the tracing group gid onto the client socket. + * Set the tracing group gid onto the client socket. + * + * Race window between mkdir and chown is OK because we are going from + * less permissive (root.root) to more permissive (root.tracing). */ static int set_permissions(void) { @@ -1047,7 +1061,7 @@ int main(int argc, char **argv) /* We do not goto error because we must not * cleanup() because a daemon is already running. */ - return EXIT_FAILURE; + exit(EXIT_FAILURE); } if (set_signal_handler() < 0) { @@ -1094,10 +1108,9 @@ int main(int argc, char **argv) } cleanup(); - return 0; + exit(EXIT_SUCCESS); error: cleanup(); - - return EXIT_FAILURE; + exit(EXIT_FAILURE); } diff --git a/ltt-sessiond/session.h b/ltt-sessiond/session.h index 96ecc217e..11f9b9ace 100644 --- a/ltt-sessiond/session.h +++ b/ltt-sessiond/session.h @@ -22,6 +22,11 @@ #include #include +/* + * FIXME: create a cmd_context structure to pass this kind of + * information around as parameter. Will facilitate multithreaded design + * later. + */ extern struct ltt_session *current_session; /* Global session list */ diff --git a/ltt-sessiond/trace.c b/ltt-sessiond/trace.c index a9ede93eb..92f8d48ce 100644 --- a/ltt-sessiond/trace.c +++ b/ltt-sessiond/trace.c @@ -145,7 +145,10 @@ int ust_create_trace(int sock, pid_t pid) cds_list_add(&trace->list, ¤t_session->ust_traces); current_session->ust_trace_count++; } + return 0; +error_create: + free(trace); error: return ret; } diff --git a/lttng/lttng.c b/lttng/lttng.c index 4c74d21fe..ab7f67ec5 100644 --- a/lttng/lttng.c +++ b/lttng/lttng.c @@ -121,6 +121,7 @@ static int process_client_opt(void) */ if (opt_trace_kernel) { ERR("Not implemented yet"); + ret = -ENOSYS; goto end; } @@ -156,9 +157,7 @@ static int process_client_opt(void) end: ERR("%s", lttng_get_readable_code(ret)); - return ret; - -error: +error: /* fall through */ return ret; } @@ -481,20 +480,19 @@ static int spawn_sessiond(char *pathname) MSG("Spawning session daemon"); pid = fork(); if (pid == 0) { - /* Spawn session daemon and tell + /* + * Spawn session daemon and tell * it to signal us when ready. */ - ret = execlp(pathname, "ltt-sessiond", "--sig-parent", "--quiet", NULL); - if (ret < 0) { - if (errno == ENOENT) { - ERR("No session daemon found. Use --sessiond-path."); - } else { - perror("execlp"); - } - kill(getppid(), SIGTERM); - exit(EXIT_FAILURE); + execlp(pathname, "ltt-sessiond", "--sig-parent", "--quiet", NULL); + /* execlp only returns if error happened */ + if (errno == ENOENT) { + ERR("No session daemon found. Use --sessiond-path."); + } else { + perror("execlp"); } - exit(EXIT_SUCCESS); + kill(getppid(), SIGTERM); /* unpause parent */ + exit(EXIT_FAILURE); } else if (pid > 0) { /* Wait for ltt-sessiond to start */ pause(); @@ -519,7 +517,7 @@ end: static int check_ltt_sessiond(void) { int ret; - char *pathname = NULL; + char *pathname = NULL, *alloc_pathname = NULL; ret = lttng_check_session_daemon(); if (ret < 0) { @@ -534,24 +532,19 @@ static int check_ltt_sessiond(void) } else { /* Try LTTNG_SESSIOND_PATH env variable */ pathname = getenv(LTTNG_SESSIOND_PATH_ENV); - if (pathname != NULL) { - /* strdup here in order to make the free() - * not fail later on. - */ - pathname = strdup(pathname); - } } /* Let's rock and roll */ if (pathname == NULL) { - ret = asprintf(&pathname, "ltt-sessiond"); + ret = asprintf(&alloc_pathname, "ltt-sessiond"); if (ret < 0) { goto end; } + pathname = alloc_pathname; } ret = spawn_sessiond(pathname); - free(pathname); + free(alloc_pathname); if (ret < 0) { ERR("Problem occurs when starting %s", pathname); goto end; diff --git a/lttng/options.c b/lttng/options.c index 57cb9b795..279bc2ffd 100644 --- a/lttng/options.c +++ b/lttng/options.c @@ -41,7 +41,7 @@ int opt_stop_trace = 0; pid_t opt_trace_pid = 0; enum { - OPT_HELP = 42, + OPT_HELP, OPT_CREATE_SESSION, OPT_DESTROY_SESSION, }; @@ -91,7 +91,7 @@ static void usage(FILE *ofp) fprintf(ofp, " -c, --create-session NAME Create a new session\n"); fprintf(ofp, " -l, --list-sessions List all available sessions\n"); fprintf(ofp, " -s, --session UUID Specify tracing session using UUID\n"); - fprintf(ofp, " -d, --destroy-session=NAME Destroy the session specified by NAME\n"); + fprintf(ofp, " -d, --destroy-session NAME Destroy the session specified by NAME\n"); fprintf(ofp, "\n"); fprintf(ofp, "Tracing options:\n"); //fprintf(ofp, " --kernel Enable kernel tracing\n"); @@ -104,7 +104,7 @@ static void usage(FILE *ofp) fprintf(ofp, " --stop Stop tracing\n"); fprintf(ofp, "\n"); fprintf(ofp, "Please see the lttng(1) man page for full documentation.\n"); - fprintf(ofp, "See http://lttng.org/ust for updates, bug reports and news.\n"); + fprintf(ofp, "See http://lttng.org for updates, bug reports and news.\n"); } /* -- 2.34.1