[lttng-dev] [PATCH 1/2] Fix: read subbuffer mmap signedness issue

[Mathieu Desnoyers]

----- Original Message -----
> From: "David Goulet" <dgoulet at efficios.com>
> To: lttng-dev at lists.lttng.org
> Sent: Friday, February 7, 2014 3:42:39 PM
> Subject: [lttng-dev] [PATCH 1/2] Fix: read subbuffer mmap signedness issue
> 
> Signed and unsigned values were compared thus making the code path where
> ret = -1 being interpreted as ret > len == true thus not cleaning up the
> relayd.
> 
> This patch simplifies the read subbuffer mmap operation since
> lttng_write() now provides a guarantee that the return data is either
> equal to the count passed or less which the later means the endpoint has
> stop working.
> 
> Signed-off-by: David Goulet <dgoulet at efficios.com>
> ---
>  src/common/consumer.c | 82
>  +++++++++++++++++++++++++++++----------------------
>  1 file changed, 47 insertions(+), 35 deletions(-)
> 
> diff --git a/src/common/consumer.c b/src/common/consumer.c
> index 36ec024..59d4366 100644
> --- a/src/common/consumer.c
> +++ b/src/common/consumer.c
> @@ -1451,7 +1451,7 @@ ssize_t lttng_consumer_on_read_subbuffer_mmap(
>  {
>  	unsigned long mmap_offset;
>  	void *mmap_base;
> -	ssize_t ret = 0, written = 0;
> +	ssize_t ret = 0, written = 0, signed_len;
>  	off_t orig_offset = stream->out_fd_offset;
>  	/* Default is on the disk */
>  	int outfd = stream->out_fd;
> @@ -1584,44 +1584,56 @@ ssize_t lttng_consumer_on_read_subbuffer_mmap(
>  		}
>  	}
>  
> -	while (len > 0) {
> -		ret = lttng_write(outfd, mmap_base + mmap_offset, len);
> -		DBG("Consumer mmap write() ret %zd (len %lu)", ret, len);
> -		if (ret < len) {
> -			/*
> -			 * This is possible if the fd is closed on the other side (outfd)
> -			 * or any write problem. It can be verbose a bit for a normal
> -			 * execution if for instance the relayd is stopped abruptly. This
> -			 * can happen so set this to a DBG statement.
> -			 */
> -			DBG("Error in file write mmap");
> -			if (written == 0) {
> -				written = -errno;
> -			}
> -			/* Socket operation failed. We consider the relayd dead */
> -			if (errno == EPIPE || errno == EINVAL) {
> -				relayd_hang_up = 1;
> -				goto write_error;
> -			}
> -			goto end;
> -		} else if (ret > len) {
> -			PERROR("Error in file write (ret %zd > len %lu)", ret, len);
> -			written += ret;
> -			goto end;
> +	/*
> +	 * Set the len value to a signed value so the if/else statement can
> +	 * evaluate it against the signed returned value from lttng_write since
> +	 * -1 can be returned indicating a communication failure.
> +	 */
> +	signed_len = len;
> +
> +	/*
> +	 * This call guarantee that len or less is returned. It's impossible to
> +	 * receive a ret value that is bigger than len.
> +	 */
> +	ret = lttng_write(outfd, mmap_base + mmap_offset, len);
> +	DBG("Consumer mmap write() ret %zd (len %lu)", ret, len);
> +	if (ret != signed_len) {
> +		/*
> +		 * Report error to caller if nothing was written else at least send the
> +		 * amount written.
> +		 */
> +		if (ret < 0) {

We should do the comparison ret < 0 first, before doing the ret vs len comparison.

If you do that, you don't semantically need the signed_len. You can then presume that
the ret value is >= 0. So you can simply cast that value in the comparison that
follows, e.g.:   if ((ssize_t) ret != len) ....

Thanks,

Mathieu

> +			written = -errno;
>  		} else {
> -			len -= ret;
> -			mmap_offset += ret;
> +			written = ret;
>  		}
>  
> -		/* This call is useless on a socket so better save a syscall. */
> -		if (!relayd) {
> -			/* This won't block, but will start writeout asynchronously */
> -			lttng_sync_file_range(outfd, stream->out_fd_offset, ret,
> -					SYNC_FILE_RANGE_WRITE);
> -			stream->out_fd_offset += ret;
> +		/* Socket operation failed. We consider the relayd dead */
> +		if (errno == EPIPE || errno == EINVAL) {
> +			/*
> +			 * This is possible if the fd is closed on the other side
> +			 * (outfd) or any write problem. It can be verbose a bit for a
> +			 * normal execution if for instance the relayd is stopped
> +			 * abruptly. This can happen so set this to a DBG statement.
> +			 */
> +			DBG("Consumer mmap write detected relayd hang up");
> +			relayd_hang_up = 1;
> +			goto write_error;
>  		}
> -		stream->output_written += ret;
> -		written += ret;
> +
> +		/* Unhandled error, print it and stop function right now. */
> +		PERROR("Error in write mmap (ret %zd < len %lu)", ret, len);
> +		goto end;
> +	}
> +	stream->output_written += ret;
> +	written = ret;
> +
> +	/* This call is useless on a socket so better save a syscall. */
> +	if (!relayd) {
> +		/* This won't block, but will start writeout asynchronously */
> +		lttng_sync_file_range(outfd, stream->out_fd_offset, len,
> +				SYNC_FILE_RANGE_WRITE);
> +		stream->out_fd_offset += len;
>  	}
>  	lttng_consumer_sync_trace_file(stream, orig_offset);
>  
> --
> 1.8.5.3
> 
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> 

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