Fix session list locking

The session list lock was not used correctly when a client requested the
session list. The session counter and iteration over the list is
protected by a single critical section now.

Move get_lttng_sessions to the main.c in order to remove lttng.h
dependency to session.c/.h and to make easier for the session list
locking.

Remove the get_session_count which is useless since the counter is now
in the session list structure. Access the counter by locking the list
and reading 'count'.

This adds two function to lock and unlock the session list.

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

diff --git a/ltt-sessiond/main.c b/ltt-sessiond/main.c
index 73f80599c06b3491acedb298fd7e1d37d4cd569c..8a808236878edbefdd468967386e57d6dc2d13f3 100644 (file)
--- a/ltt-sessiond/main.c
+++ b/ltt-sessiond/main.c
@@ -468,7 +468,7 @@ static int update_kernel_pollfd(void)
        DBG("Updating kernel_pollfd");
 
        /* Get the number of channel of all kernel session */
-       pthread_mutex_lock(&session_list_ptr->lock);
+       lock_session_list();
        cds_list_for_each_entry(session, &session_list_ptr->head, list) {
                lock_session(session);
                if (session->kernel_session == NULL) {
@@ -505,7 +505,7 @@ static int update_kernel_pollfd(void)
                }
                unlock_session(session);
        }
-       pthread_mutex_unlock(&session_list_ptr->lock);
+       unlock_session_list();
 
        /* Adding wake up pipe */
        kernel_pollfd[nb_fd - 2].fd = kernel_poll_pipe[0];
@@ -517,7 +517,7 @@ static int update_kernel_pollfd(void)
        return nb_fd;
 
 error:
-       pthread_mutex_unlock(&session_list_ptr->lock);
+       unlock_session_list();
        return -1;
 }
 
@@ -537,7 +537,7 @@ static int update_kernel_stream(int fd)
 
        DBG("Updating kernel streams for channel fd %d", fd);
 
-       pthread_mutex_lock(&session_list_ptr->lock);
+       lock_session_list();
        cds_list_for_each_entry(session, &session_list_ptr->head, list) {
                lock_session(session);
                if (session->kernel_session == NULL) {
@@ -568,10 +568,10 @@ static int update_kernel_stream(int fd)
        }
 
 end:
+       unlock_session_list();
        if (session) {
                unlock_session(session);
        }
-       pthread_mutex_unlock(&session_list_ptr->lock);
        return ret;
 }
 
@@ -1227,6 +1227,30 @@ error:
        return ret;
 }
 
+/*
+ * Using the session list, filled a lttng_session array to send back to the
+ * client for session listing.
+ *
+ * The session list lock MUST be acquired before calling this function. Use
+ * lock_session_list() and unlock_session_list().
+ */
+static void list_lttng_sessions(struct lttng_session *sessions)
+{
+       int i = 0;
+       struct ltt_session *session;
+
+       DBG("Getting all available session");
+       /*
+        * Iterate over session list and append data after the control struct in
+        * the buffer.
+        */
+       cds_list_for_each_entry(session, &session_list_ptr->head, list) {
+               strncpy(sessions[i].path, session->path, PATH_MAX);
+               strncpy(sessions[i].name, session->name, NAME_MAX);
+               i++;
+       }
+}
+
 /*
  *     process_client_msg
  *
@@ -1938,20 +1962,23 @@ static int process_client_msg(struct command_ctx *cmd_ctx)
        */
        case LTTNG_LIST_SESSIONS:
        {
-               unsigned int session_count;
+               lock_session_list();
 
-               session_count = get_session_count();
-               if (session_count == 0) {
+               if (session_list_ptr->count == 0) {
                        ret = LTTCOMM_NO_SESSION;
                        goto error;
                }
 
-               ret = setup_lttng_msg(cmd_ctx, sizeof(struct lttng_session) * session_count);
+               ret = setup_lttng_msg(cmd_ctx, sizeof(struct lttng_session) *
+                               session_list_ptr->count);
                if (ret < 0) {
                        goto setup_error;
                }
 
-               get_lttng_session((struct lttng_session *)(cmd_ctx->llm->payload));
+               /* Filled the session array */
+               list_lttng_sessions((struct lttng_session *)(cmd_ctx->llm->payload));
+
+               unlock_session_list();
 
                ret = LTTCOMM_OK;
                break;

diff --git a/ltt-sessiond/session.c b/ltt-sessiond/session.c
index 97ab098dca3c93c102cc7042619783ec9dec02ec..58518346b434fcdc4ba96b005b7ed888cdbd14d0 100644 (file)
--- a/ltt-sessiond/session.c
+++ b/ltt-sessiond/session.c
@@ -17,6 +17,7 @@
  */
 
 #define _GNU_SOURCE
+#include <limits.h>
 #include <pthread.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -87,35 +88,35 @@ struct ltt_session_list *get_session_list(void)
 }
 
 /*
- * Acquire session lock
+ * Acquire session list lock
  */
-void lock_session(struct ltt_session *session)
+void lock_session_list(void)
 {
-       pthread_mutex_lock(&session->lock);
+       pthread_mutex_lock(&ltt_session_list.lock);
 }
 
 /*
- * Release session lock
+ * Release session list lock
  */
-void unlock_session(struct ltt_session *session)
+void unlock_session_list(void)
 {
-       pthread_mutex_unlock(&session->lock);
+       pthread_mutex_unlock(&ltt_session_list.lock);
 }
 
 /*
- *  get_session_count
- *
- *  Return session_count
+ * Acquire session lock
  */
-unsigned int get_session_count(void)
+void lock_session(struct ltt_session *session)
 {
-       unsigned int count;
-
-       pthread_mutex_lock(&ltt_session_list.lock);
-       count = ltt_session_list.count;
-       pthread_mutex_unlock(&ltt_session_list.lock);
+       pthread_mutex_lock(&session->lock);
+}
 
-       return count;
+/*
+ * Release session lock
+ */
+void unlock_session(struct ltt_session *session)
+{
+       pthread_mutex_unlock(&session->lock);
 }
 
 /*
@@ -129,14 +130,14 @@ struct ltt_session *find_session_by_name(char *name)
        int found = 0;
        struct ltt_session *iter;
 
-       pthread_mutex_lock(&ltt_session_list.lock);
+       lock_session_list();
        cds_list_for_each_entry(iter, &ltt_session_list.head, list) {
                if (strncmp(iter->name, name, strlen(name)) == 0) {
                        found = 1;
                        break;
                }
        }
-       pthread_mutex_unlock(&ltt_session_list.lock);
+       unlock_session_list();
 
        if (!found) {
                iter = NULL;
@@ -157,7 +158,7 @@ int destroy_session(char *name)
        int found = -1;
        struct ltt_session *iter;
 
-       pthread_mutex_lock(&ltt_session_list.lock);
+       lock_session_list();
        cds_list_for_each_entry(iter, &ltt_session_list.head, list) {
                if (strcmp(iter->name, name) == 0) {
                        DBG("Destroying session %s", iter->name);
@@ -170,7 +171,7 @@ int destroy_session(char *name)
                        break;
                }
        }
-       pthread_mutex_unlock(&ltt_session_list.lock);
+       unlock_session_list();
 
        return found;
 }
@@ -244,9 +245,9 @@ int create_session(char *name, char *path)
        new_session->ust_trace_count = 0;
 
        /* Add new session to the session list */
-       pthread_mutex_lock(&ltt_session_list.lock);
+       lock_session_list();
        add_session_list(new_session);
-       pthread_mutex_unlock(&ltt_session_list.lock);
+       unlock_session_list();
 
        /* Init lock */
        pthread_mutex_init(&new_session->lock, NULL);
@@ -265,35 +266,3 @@ error_exist:
 error_malloc:
        return ret;
 }
-
-/*
- *  get_lttng_session
- *
- *  Iterate over the global session list and fill the lttng_session array.
- */
-void get_lttng_session(struct lttng_session *sessions)
-{
-       int i = 0;
-       struct ltt_session *iter;
-       struct lttng_session lsess;
-
-       DBG("Getting all available session");
-
-       /*
-        * Iterate over session list and append data after the control struct in
-        * the buffer.
-        */
-       pthread_mutex_lock(&ltt_session_list.lock);
-       cds_list_for_each_entry(iter, &ltt_session_list.head, list) {
-               strncpy(lsess.path, iter->path, sizeof(lsess.path));
-               lsess.path[sizeof(lsess.path) - 1] = '\0';
-               strncpy(lsess.name, iter->name, sizeof(lsess.name));
-               lsess.name[sizeof(lsess.name) - 1] = '\0';
-               memcpy(&sessions[i], &lsess, sizeof(lsess));
-               i++;
-               /* Reset struct for next pass */
-               memset(&lsess, 0, sizeof(lsess));
-       }
-       pthread_mutex_unlock(&ltt_session_list.lock);
-}
-

diff --git a/ltt-sessiond/session.h b/ltt-sessiond/session.h
index 9eaa1c535e783897f0ed0ce90bf806b89f7e5e30..837b9062d35a80d364a7d3ee7b6492ecf073400e 100644 (file)
--- a/ltt-sessiond/session.h
+++ b/ltt-sessiond/session.h
@@ -19,7 +19,7 @@
 #ifndef _LTT_SESSION_H
 #define _LTT_SESSION_H
 
-#include <lttng/lttng.h>
+//#include <lttng/lttng.h>
 #include <urcu/list.h>
 
 /*
@@ -37,10 +37,14 @@ struct ltt_session_list {
         * actions on that list.
         */
        pthread_mutex_t lock;
+
        /*
-        * Number of element in the list.
+        * Number of element in the list. The session list lock MUST be acquired if
+        * this counter is used when iterating over the session list.
         */
        unsigned int count;
+
+       /* Linked list head */
        struct cds_list_head head;
 };
 
@@ -66,11 +70,13 @@ struct ltt_session {
 /* Prototypes */
 int create_session(char *name, char *path);
 int destroy_session(char *name);
-void get_lttng_session(struct lttng_session *sessions);
+
 void lock_session(struct ltt_session *session);
+void lock_session_list(void);
 void unlock_session(struct ltt_session *session);
+void unlock_session_list(void);
+
 struct ltt_session *find_session_by_name(char *name);
-unsigned int get_session_count(void);
 struct ltt_session_list *get_session_list(void);
 
 #endif /* _LTT_SESSION_H */