[lttng-dev] [PATCH lttng-tools v3] Fix: timeout for connect is not respected

[Jonathan Rajotte]

Problem:

The previous algorithm was not *wrong* but was prone to C type conversion
error.

Previous algorithm:

((A.sec - B.sec) * MSEC_PER_SEC) + ((A.nsec - B.nsec) / NSEC_PER_SEC)

((A.sec - B.sec) * 1000000ULL) + ((A.nsec - B.nsec) / 1000000ULL)

Note that "(A.nsec - B.nsec)" can be negative.

A *quick* fix here is to cast the NSEC_PER_SEC to long allowing a valid
division operation.

The problem was introduced by 395d6b02dda3db1acd08936f49c1dc8efc48e613.

Solution:

Perform the time diff using a temporary timespec struct and perform the
carry operation manually. This prevent negative value for nsec and help
prevent C type promotion problems.

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
---

Implement difference without posing the hypothesis that time_a is necessarily
bigger than time_b.

Fixed type in comment.

Added an assert statement to ensure premises before returning. Feel free to
remove.

 src/common/sessiond-comm/inet.c  | 32 ++++++++++++++++++++++++--------
 src/common/sessiond-comm/inet6.c | 32 ++++++++++++++++++++++++--------
 2 files changed, 48 insertions(+), 16 deletions(-)

diff --git a/src/common/sessiond-comm/inet.c b/src/common/sessiond-comm/inet.c
index e0b3e7a96..f03fd1fb5 100644
--- a/src/common/sessiond-comm/inet.c
+++ b/src/common/sessiond-comm/inet.c
@@ -119,16 +119,32 @@ static
 unsigned long time_diff_ms(struct timespec *time_a,
 		struct timespec *time_b)
 {
-	time_t sec_diff;
-	long nsec_diff;
-	unsigned long result_ms;
+	struct timespec diff;
 
-	sec_diff = time_a->tv_sec - time_b->tv_sec;
-	nsec_diff = time_a->tv_nsec - time_b->tv_nsec;
+	if ((time_a->tv_sec > time_b->tv_sec)
+		|| ((time_a->tv_sec == time_b->tv_sec)
+			&& (time_a->tv_nsec > time_b->tv_nsec))) {
+		/* time_a is bigger */
+		diff.tv_sec = time_a->tv_sec - time_b->tv_sec;
+		diff.tv_nsec = time_a->tv_nsec - time_b->tv_nsec;
+	} else {
+		/* time_b is bigger */
+		diff.tv_sec = time_b->tv_sec - time_a->tv_sec;
+		diff.tv_nsec = time_b->tv_nsec - time_a->tv_nsec;
+	}
 
-	result_ms = sec_diff * MSEC_PER_SEC;
-	result_ms += nsec_diff / NSEC_PER_MSEC;
-	return result_ms;
+	if (diff.tv_nsec < 0) {
+		/* Perform borrow operation */
+		diff.tv_sec -= 1;
+		diff.tv_nsec += NSEC_PER_SEC;
+	}
+
+	/*
+	 * Transform into MSEC, diff.tv_nsec is always positive due to borrow
+	 * operation. diff.tv_sec is always positive.
+	 */
+	assert(diff.tv_sec >= 0 && diff.tv_nsec >= 0);
+	return (diff.tv_sec * MSEC_PER_SEC) + (diff.tv_nsec / NSEC_PER_MSEC);
 }
 
 static
diff --git a/src/common/sessiond-comm/inet6.c b/src/common/sessiond-comm/inet6.c
index dfb5fc5d1..beb2dd9ed 100644
--- a/src/common/sessiond-comm/inet6.c
+++ b/src/common/sessiond-comm/inet6.c
@@ -117,16 +117,32 @@ static
 unsigned long time_diff_ms(struct timespec *time_a,
 		struct timespec *time_b)
 {
-	time_t sec_diff;
-	long nsec_diff;
-	unsigned long result_ms;
+	struct timespec diff;
 
-	sec_diff = time_a->tv_sec - time_b->tv_sec;
-	nsec_diff = time_a->tv_nsec - time_b->tv_nsec;
+	if ((time_a->tv_sec > time_b->tv_sec)
+		|| ((time_a->tv_sec == time_b->tv_sec)
+			&& (time_a->tv_nsec > time_b->tv_nsec))) {
+		/* time_a is bigger */
+		diff.tv_sec = time_a->tv_sec - time_b->tv_sec;
+		diff.tv_nsec = time_a->tv_nsec - time_b->tv_nsec;
+	} else {
+		/* time_b is bigger */
+		diff.tv_sec = time_b->tv_sec - time_a->tv_sec;
+		diff.tv_nsec = time_b->tv_nsec - time_a->tv_nsec;
+	}
 
-	result_ms = sec_diff * MSEC_PER_SEC;
-	result_ms += nsec_diff / NSEC_PER_MSEC;
-	return result_ms;
+	if (diff.tv_nsec < 0) {
+		/* Perform borrow operation */
+		diff.tv_sec -= 1;
+		diff.tv_nsec += NSEC_PER_SEC;
+	}
+
+	/*
+	 * Transform into MSEC, diff.tv_nsec is always positive due to borrow
+	 * operation. diff.tv_sec is always positive.
+	 */
+	assert(diff.tv_sec >= 0 && diff.tv_nsec >= 0);
+	return (diff.tv_sec * MSEC_PER_SEC) + (diff.tv_nsec / NSEC_PER_MSEC);
 }
 
 static
-- 
2.17.1