[LTTNG-TOOLS 2.3 PATCH 2/2] Test: enable kernel events after start

[LTTNG-TOOLS 2.3 PATCH 2/2] Test: enable kernel events after start

[Julien Desfossez]

This test detects if we actually append new metadata when enabling a
kernel event after a start.

Signed-off-by: Julien Desfossez <jdesfossez@efficios.com>
---
 tests/regression/kernel/test_event_basic |   24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/tests/regression/kernel/test_event_basic b/tests/regression/kernel/test_event_basic
index 5c19744..a182f9f 100755
--- a/tests/regression/kernel/test_event_basic
+++ b/tests/regression/kernel/test_event_basic
@@ -19,7 +19,7 @@ TEST_DESC="Kernel tracer - Basic event"
 
 CURDIR=$(dirname $0)/
 TESTDIR=$CURDIR/../..
-NUM_TESTS=12
+NUM_TESTS=20
 
 source $TESTDIR/utils/utils.sh
 
@@ -46,6 +46,27 @@ function test_event_basic()
 	rm -rf $TRACE_PATH
 }
 
+function test_enable_after_start()
+{
+	TRACE_PATH=$(mktemp -d)
+	SESSION_NAME="kernel_enable_after_start"
+
+	create_lttng_session $SESSION_NAME $TRACE_PATH
+
+	lttng_enable_kernel_event $SESSION_NAME "sched_switch"
+
+	start_lttng_tracing
+	lttng_enable_kernel_event $SESSION_NAME "sched_process_exit"
+	stop_lttng_tracing
+
+	validate_trace "sched_switch" $TRACE_PATH
+	validate_trace "sched_process_exit" $TRACE_PATH
+
+	destroy_lttng_session $SESSION_NAME
+
+	rm -rf $TRACE_PATH
+}
+
 # MUST set TESTDIR before calling those functions
 plan_tests $NUM_TESTS
 
@@ -62,6 +83,7 @@ skip $isroot "Root access is needed. Skipping all tests." $NUM_TESTS ||
 	start_lttng_sessiond
 
 	test_event_basic
+	test_enable_after_start
 
 	stop_lttng_sessiond
 }
-- 
1.7.10.4

Re: [LTTNG-TOOLS 2.3 PATCH 1/2] Fix: return code of get_subbuff on metadata stream

[Mathieu Desnoyers]

* Julien Desfossez (jdesfossez@efficios.com) wrote:
> If no metadata is available on the kernel metadata stream when we
> do a get_subbuff, the kernel returns -EPERM, the consumer was not
> checking for this return code and closed the stream prematurely. It
> worked if no new metadata was added during the session.

Is the proper fix to change lttng-tools or lttng-modules ?

I would rather think that lttng-modules should return -ENODATA rather
than -EPERM in this case. It's clearly not a permission issue.

Thoughts ?

Thanks,

Mathieu

> 
> Signed-off-by: Julien Desfossez <jdesfossez@efficios.com>
> ---
>  src/common/consumer.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/common/consumer.c b/src/common/consumer.c
> index 4b657f3..a070f36 100644
> --- a/src/common/consumer.c
> +++ b/src/common/consumer.c
> @@ -2288,7 +2288,8 @@ restart:
>  				} while (len > 0);
>  
>  				/* It's ok to have an unavailable sub-buffer */
> -				if (len < 0 && len != -EAGAIN && len != -ENODATA) {
> +				if (len < 0 && len != -EAGAIN && len != -ENODATA &&
> +						len != -EPERM) {
>  					/* Clean up stream from consumer and free it. */
>  					lttng_poll_del(&events, stream->wait_fd);
>  					consumer_del_metadata_stream(stream, metadata_ht);
> -- 
> 1.7.10.4
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [LTTNG-TOOLS 2.3 PATCH 1/2] Fix: return code of get_subbuff on metadata stream

[Mathieu Desnoyers]

* Julien Desfossez (jdesfossez@efficios.com) wrote:
> On error, the ioctl kernctl_get_next_subbuf returns -1 and sets errno to
> a meaningful value but we were ignoring it. It was causing
> lttng_kconsumer_read_subbuffer to return -1 (error) instead of -EAGAIN
> (normal).

Merging an updated patch instead:

commit 56591bac20c0f3b728c95d92702d243de838bdc4
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date:   Mon Aug 5 13:46:58 2013 -0400

    Fix: kernel ctl error codes are based on errno
    
    ioctl returns -1, and error codes are based on -errno.
    
    Signed-off-by: Julien Desfossez <jdesfossez@efficios.com>
    Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Thanks,

Mathieu

> 
> Signed-off-by: Julien Desfossez <jdesfossez@efficios.com>
> ---
>  src/common/kernel-consumer/kernel-consumer.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/common/kernel-consumer/kernel-consumer.c b/src/common/kernel-consumer/kernel-consumer.c
> index 3086abe..f8978d2 100644
> --- a/src/common/kernel-consumer/kernel-consumer.c
> +++ b/src/common/kernel-consumer/kernel-consumer.c
> @@ -851,7 +851,7 @@ ssize_t lttng_kconsumer_read_subbuffer(struct lttng_consumer_stream *stream,
>  	/* Get the next subbuffer */
>  	err = kernctl_get_next_subbuf(infd);
>  	if (err != 0) {
> -		ret = err;
> +		ret = -errno;
>  		/*
>  		 * This is a debug message even for single-threaded consumer,
>  		 * because poll() have more relaxed criterions than get subbuf,
> -- 
> 1.7.10.4
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

[LTTNG-TOOLS 2.3 PATCH 1/2] Fix: return code of get_subbuff on metadata stream

[Julien Desfossez]

On error, the ioctl kernctl_get_next_subbuf returns -1 and sets errno to
a meaningful value but we were ignoring it. It was causing
lttng_kconsumer_read_subbuffer to return -1 (error) instead of -EAGAIN
(normal).

Signed-off-by: Julien Desfossez <jdesfossez@efficios.com>
---
 src/common/kernel-consumer/kernel-consumer.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/common/kernel-consumer/kernel-consumer.c b/src/common/kernel-consumer/kernel-consumer.c
index 3086abe..f8978d2 100644
--- a/src/common/kernel-consumer/kernel-consumer.c
+++ b/src/common/kernel-consumer/kernel-consumer.c
@@ -851,7 +851,7 @@ ssize_t lttng_kconsumer_read_subbuffer(struct lttng_consumer_stream *stream,
 	/* Get the next subbuffer */
 	err = kernctl_get_next_subbuf(infd);
 	if (err != 0) {
-		ret = err;
+		ret = -errno;
 		/*
 		 * This is a debug message even for single-threaded consumer,
 		 * because poll() have more relaxed criterions than get subbuf,
-- 
1.7.10.4

[LTTNG-TOOLS 2.3 PATCH 1/2] Fix: return code of get_subbuff on metadata stream

[Julien Desfossez]

If no metadata is available on the kernel metadata stream when we
do a get_subbuff, the kernel returns -EPERM, the consumer was not
checking for this return code and closed the stream prematurely. It
worked if no new metadata was added during the session.

Signed-off-by: Julien Desfossez <jdesfossez@efficios.com>
---
 src/common/consumer.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/common/consumer.c b/src/common/consumer.c
index 4b657f3..a070f36 100644
--- a/src/common/consumer.c
+++ b/src/common/consumer.c
@@ -2288,7 +2288,8 @@ restart:
 				} while (len > 0);
 
 				/* It's ok to have an unavailable sub-buffer */
-				if (len < 0 && len != -EAGAIN && len != -ENODATA) {
+				if (len < 0 && len != -EAGAIN && len != -ENODATA &&
+						len != -EPERM) {
 					/* Clean up stream from consumer and free it. */
 					lttng_poll_del(&events, stream->wait_fd);
 					consumer_del_metadata_stream(stream, metadata_ht);
-- 
1.7.10.4