Fix: relayd: session trace chunk is copied too late

In per-pid buffering mode, a viewer can attach to a session while it
is active and find it has been closed by the time it requests new
streams. The viewer session's trace chunk is created as a side-effect
of the "get_new_streams" command and can find that the relay_session's
trace chunk has now been closed, causing the creation of the viewer
session trace chunk to fail.

This results in an unexpected error being reported to the live client.
This fix moves the creation of the viewer session's trace chunk to the
"attach" command. If the creation fails, the session is reported as
being "unknown".

Reported-by: Francis Deslauriers <francis.deslauriers@efficios.com>
Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
Tested-by: Francis Deslauriers <francis.deslauriers@efficios.com>

diff --git a/src/bin/lttng-relayd/live.c b/src/bin/lttng-relayd/live.c
index 9a3d78ef0fc3a0632476d131202b325d77e17e82..3ca1797ed9b07b12468840f0bcd963709f72a92b 100644 (file)
--- a/src/bin/lttng-relayd/live.c
+++ b/src/bin/lttng-relayd/live.c
@@ -291,6 +291,13 @@ static int make_viewer_streams(struct relay_session *session,
        assert(session);
        ASSERT_LOCKED(session->lock);
 
+       if (!viewer_trace_chunk) {
+               ERR("Internal error: viewer session associated with session \"%s\" has a NULL trace chunk",
+                               session->session_name);
+               ret = -1;
+               goto error;
+       }
+
        if (session->connection_closed) {
                *closed = true;
        }
@@ -378,6 +385,7 @@ static int make_viewer_streams(struct relay_session *session,
 
 error_unlock:
        rcu_read_unlock();
+error:
        return ret;
 }
 
@@ -959,25 +967,6 @@ int viewer_get_new_streams(struct relay_connection *conn)
        }
 
        pthread_mutex_lock(&session->lock);
-       if (!session->current_trace_chunk) {
-               /*
-                * Means the session is being destroyed. React the same way
-                * as if it could not be found at all.
-                */
-               DBG("Relay session %" PRIu64 " has no current trace chunk, replying LTTNG_VIEWER_NEW_STREAMS_ERR",
-                               session_id);
-               response.status = htobe32(LTTNG_VIEWER_NEW_STREAMS_ERR);
-               goto send_reply_unlock;
-       }
-
-       if (!conn->viewer_session->current_trace_chunk &&
-                       session->current_trace_chunk) {
-               ret = viewer_session_set_trace_chunk(conn->viewer_session,
-                               session->current_trace_chunk);
-               if (ret) {
-                       goto error_unlock_session;
-               }
-       }
        ret = make_viewer_streams(session,
                        conn->viewer_session->current_trace_chunk,
                        LTTNG_VIEWER_SEEK_LAST, &nb_total, &nb_unsent,
@@ -1057,6 +1046,7 @@ int viewer_attach_session(struct relay_connection *conn)
        struct lttng_viewer_attach_session_request request;
        struct lttng_viewer_attach_session_response response;
        struct relay_session *session = NULL;
+       enum lttng_viewer_attach_return_code viewer_attach_status;
        bool closed = false;
        uint64_t session_id;
 
@@ -1106,10 +1096,10 @@ int viewer_attach_session(struct relay_connection *conn)
        }
 
        send_streams = 1;
-       ret = viewer_session_attach(conn->viewer_session, session);
-       if (ret) {
-               DBG("Already a viewer attached");
-               response.status = htobe32(LTTNG_VIEWER_ATTACH_ALREADY);
+       viewer_attach_status = viewer_session_attach(conn->viewer_session,
+                       session);
+       if (viewer_attach_status != LTTNG_VIEWER_ATTACH_OK) {
+               response.status = htobe32(viewer_attach_status);
                goto send_reply;
        }
 
@@ -1126,14 +1116,6 @@ int viewer_attach_session(struct relay_connection *conn)
                goto send_reply;
        }
 
-       if (!conn->viewer_session->current_trace_chunk &&
-                       session->current_trace_chunk) {
-               ret = viewer_session_set_trace_chunk(conn->viewer_session,
-                               session->current_trace_chunk);
-               if (ret) {
-                       goto end_put_session;
-               }
-       }
        ret = make_viewer_streams(session,
                        conn->viewer_session->current_trace_chunk, seek_type,
                        &nb_streams, NULL, NULL, &closed);

diff --git a/src/bin/lttng-relayd/viewer-session.c b/src/bin/lttng-relayd/viewer-session.c
index 7ff58ed967842b75881a43c7de4fa5f029cc23fe..d394465ba1c51d3f1575ed7630c2f6dc4c942908 100644 (file)
--- a/src/bin/lttng-relayd/viewer-session.c
+++ b/src/bin/lttng-relayd/viewer-session.c
@@ -42,25 +42,45 @@ end:
 }
 
 /* The existence of session must be guaranteed by the caller. */
-int viewer_session_attach(struct relay_viewer_session *vsession,
+enum lttng_viewer_attach_return_code viewer_session_attach(
+               struct relay_viewer_session *vsession,
                struct relay_session *session)
 {
-       int ret = 0;
+       enum lttng_viewer_attach_return_code viewer_attach_status =
+                       LTTNG_VIEWER_ATTACH_OK;
 
        ASSERT_LOCKED(session->lock);
 
        /* Will not fail, as per the ownership guarantee. */
        if (!session_get(session)) {
-               ret = -1;
+               viewer_attach_status = LTTNG_VIEWER_ATTACH_UNK;
                goto end;
        }
        if (session->viewer_attached) {
-               ret = -1;
+               viewer_attach_status = LTTNG_VIEWER_ATTACH_ALREADY;
        } else {
+               int ret;
+
+               assert(session->current_trace_chunk);
+               assert(!vsession->current_trace_chunk);
                session->viewer_attached = true;
+
+               ret = viewer_session_set_trace_chunk(vsession,
+                               session->current_trace_chunk);
+               if (ret) {
+                       /*
+                        * The live protocol does not define a generic error
+                        * value for the "attach" command. The "unknown"
+                        * status is used so that the viewer may handle this
+                        * failure as if the session didn't exist anymore.
+                        */
+                       DBG("Failed to create a viewer trace chunk from the current trace chunk of session \"%s\", returning LTTNG_VIEWER_ATTACH_UNK",
+                                       session->session_name);
+                       viewer_attach_status = LTTNG_VIEWER_ATTACH_UNK;
+               }
        }
 
-       if (!ret) {
+       if (viewer_attach_status == LTTNG_VIEWER_ATTACH_OK) {
                pthread_mutex_lock(&vsession->session_list_lock);
                /* Ownership is transfered to the list. */
                cds_list_add_rcu(&session->viewer_session_node,
@@ -71,7 +91,7 @@ int viewer_session_attach(struct relay_viewer_session *vsession,
                session_put(session);
        }
 end:
-       return ret;
+       return viewer_attach_status;
 }
 
 /* The existence of session must be guaranteed by the caller. */

diff --git a/src/bin/lttng-relayd/viewer-session.h b/src/bin/lttng-relayd/viewer-session.h
index c127f69c1b42b18cd3b7e1e277138d4aeb6b4b1d..5c8b2e54d31ff07e93dd979a8f921b5fbaaed233 100644 (file)
--- a/src/bin/lttng-relayd/viewer-session.h
+++ b/src/bin/lttng-relayd/viewer-session.h
@@ -48,7 +48,8 @@ struct relay_viewer_session *viewer_session_create(void);
 void viewer_session_destroy(struct relay_viewer_session *vsession);
 void viewer_session_close(struct relay_viewer_session *vsession);
 
-int viewer_session_attach(struct relay_viewer_session *vsession,
+enum lttng_viewer_attach_return_code viewer_session_attach(
+               struct relay_viewer_session *vsession,
                struct relay_session *session);
 int viewer_session_is_attached(struct relay_viewer_session *vsession,
                struct relay_session *session);