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

[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

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

[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

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