Comments and cleanup review Mathieu & David

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: David Goulet <david.goulet@polymtl.ca>

diff --git a/include/lttng/lttng.h b/include/lttng/lttng.h
index 3bb245239fd6889f86a3122b9f90cc9d3733ebcb..957484f0900daf1084368d2370f2174f8b4b95cb 100644 (file)
--- 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 5cac0b7982ecef86c52dde2ca69b9ef280ee9e55..adb9a8d843572d96f62938d02daf0ea111599c89 100644 (file)
--- a/liblttsessiondcomm/liblttsessiondcomm.h
+++ b/liblttsessiondcomm/liblttsessiondcomm.h
@@ -24,6 +24,11 @@
 #include <limits.h>
 #include <uuid/uuid.h>
 
+/*
+ * 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 f5431bc0ae0299102b1d52b7c42bbc4f4b68de30..21b05fc8621847523169bae1d59d77e3bdf21391 100644 (file)
--- 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 96ecc217ee2daabd1acfcf21915afd0d868af9d2..11f9b9ace8986b0366e046d5787a7803fa38180c 100644 (file)
--- a/ltt-sessiond/session.h
+++ b/ltt-sessiond/session.h
@@ -22,6 +22,11 @@
 #include <lttng/lttng.h>
 #include <uuid/uuid.h>
 
+/*
+ * 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 a9ede93eb7b076d5273825606a1d271f10662755..92f8d48ce141960902cba89e1b939de2fab48b5b 100644 (file)
--- 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, &current_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 4c74d21fefb665172f2fac166e9de0b5ffee20c8..ab7f67ec52c296245d697d4a663488318acc22f0 100644 (file)
--- 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 57cb9b7954677153ee56e3535b510be115818c87..279bc2ffd6dd28d4593049be62c9d7db8ecb581d 100644 (file)
--- 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");
 }
 
 /*