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 could be 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
helps prevent C type promotion problems.
Signed-off-by: Jonathan Rajotte <jonathan.rajotte-jul...@efficios.com>
---
I initially misplaced the issue.
As mentioned in the commit message, we can also cast NSEC_PER_SEC to long to fix
the issue. I prefer the use of a timespec struct for expressiveness.
Also the case were we pass a time_b > time_a is handled by returning a
difference of zero. We might want to assert here since it should never happen.
src/common/sessiond-comm/inet.c | 28 ++++++++++++++++++++--------
src/common/sessiond-comm/inet6.c | 28 ++++++++++++++++++++--------
2 files changed, 40 insertions(+), 16 deletions(-)
diff --git a/src/common/sessiond-comm/inet.c b/src/common/sessiond-comm/inet.c
index e0b3e7a96..94a3c3a67 100644
--- a/src/common/sessiond-comm/inet.c
+++ b/src/common/sessiond-comm/inet.c
@@ -119,16 +119,28 @@ 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;
+ diff.tv_sec = time_a->tv_sec - time_b->tv_sec;
+ diff.tv_nsec = time_a->tv_nsec - time_b->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;
+ }
+
+ if (diff.tv_sec < 0) {
+ /* We went back in time. Put all to zero */
+ diff.tv_sec = 0;
+ diff.tv_nsec = 0;
+ }
+
+ /*
+ * Transform into MSEC, diff.tv_nsec is always positive due to borrow
+ * operation. diff.tb_sec is always positive.
+ */
+ 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..07043e93f 100644
--- a/src/common/sessiond-comm/inet6.c
+++ b/src/common/sessiond-comm/inet6.c
@@ -117,16 +117,28 @@ 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;
+ diff.tv_sec = time_a->tv_sec - time_b->tv_sec;
+ diff.tv_nsec = time_a->tv_nsec - time_b->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;
+ }
+
+ if (diff.tv_sec < 0) {
+ /* We went back in time. Put all to zero */
+ diff.tv_sec = 0;
+ diff.tv_nsec = 0;
+ }
+
+ /*
+ * Transform into MSEC, diff.tv_nsec is always positive due to borrow
+ * operation. diff.tb_sec is always positive.
+ */
+ return (diff.tv_sec * MSEC_PER_SEC) + (diff.tv_nsec / NSEC_PER_MSEC);
}
static
--
2.17.1
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
