[lttng-dev] [PATCH lttng-tools 1/2] Fix: Connect timeout arithmetic in inet/inet6 (v2)
Jonathan Rajotte-Julien
jonathan.rajotte-julien at efficios.com
Fri Nov 9 18:13:11 EST 2018
As discussed on IRC, could you make sure to test it manually at least once since
the problematic scenario is never tested by regression test.
Looks much better than my first attempt but we never know.
You can force delay locally using tc.
Something like:
LTTNG_NETWORK_SOCKET_TIMEOUT=4000 lttng-sessiond -b -vvv
lttng-relayd -b
lttng create --live
sudo tc qdisc add dev lo root netem delay 10000ms
lttng enable-event -u -a
#Should see errors of connection timeout taking around 4 secs, validate using
#the logs.
sudo tc qdisc del dev lo root netem delay 10000ms
You can also do the opposite for delays to validate working connect that take a
long time.
As Simon pointed out, unit tests would be even better for these new functions.
Something like test_utils_expand_path.c perhaps? A regression test would be nice
but I'm not sure how we would do it based on the current test framework.
Other comments inline for stuff I did not see the first time.
On Fri, Nov 09, 2018 at 05:40:46PM -0500, Mathieu Desnoyers wrote:
> The nanoseconds part of the timespec struct time_a is not always
> bigger than time_b since it wrap around each seconds.
>
> Use 64-bit arithmetic to perform the difference.
>
> Merge/move duplicated code into utils.c.
>
> This function is really doing two things. Split it into timespec_to_ms()
> and timespec_abs_diff().
>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
> ---
> Changes since v1:
> - Use NSEC_PER_SEC constant.
> ---
> src/common/common.h | 5 +++++
> src/common/sessiond-comm/inet.c | 28 ++++++++--------------------
> src/common/sessiond-comm/inet6.c | 28 ++++++++--------------------
> src/common/utils.c | 31 +++++++++++++++++++++++++++++++
> 4 files changed, 52 insertions(+), 40 deletions(-)
>
> diff --git a/src/common/common.h b/src/common/common.h
> index 41eb0361..48f39e0e 100644
> --- a/src/common/common.h
> +++ b/src/common/common.h
> @@ -19,9 +19,14 @@
> #ifndef _COMMON_H
> #define _COMMON_H
>
> +#include <time.h>
> +
> #include "error.h"
> #include "macros.h"
> #include "runas.h"
> #include "readwrite.h"
>
Given the current format of common.h, wouldn't it be better to have a
timespec_ops.h file with these declaration in it? I guess it can be done in a
later commit to minimize backporting since it is not necessary for the fix.
Please add a comment explaining behaviour on error and what it represent.
> +int timespec_to_ms(struct timespec ts, unsigned long *ms);
> +struct timespec timespec_abs_diff(struct timespec ts_a, struct timespec ts_b);
> +
> #endif /* _COMMON_H */
> diff --git a/src/common/sessiond-comm/inet.c b/src/common/sessiond-comm/inet.c
> index e0b3e7a9..76a3b7ff 100644
> --- a/src/common/sessiond-comm/inet.c
> +++ b/src/common/sessiond-comm/inet.c
> @@ -112,31 +112,13 @@ int connect_no_timeout(struct lttcomm_sock *sock)
> sizeof(sock->sockaddr.addr.sin));
> }
>
> -/*
> - * Return time_a - time_b in milliseconds.
> - */
> -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;
> -
> - sec_diff = time_a->tv_sec - time_b->tv_sec;
> - nsec_diff = 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;
> -}
> -
> static
> int connect_with_timeout(struct lttcomm_sock *sock)
> {
> unsigned long timeout = lttcomm_get_network_timeout();
> int ret, flags, connect_ret;
> struct timespec orig_time, cur_time;
> + unsigned long diff_ms;
>
> ret = fcntl(sock->fd, F_GETFL, 0);
> if (ret == -1) {
> @@ -217,7 +199,13 @@ int connect_with_timeout(struct lttcomm_sock *sock)
> connect_ret = ret;
> goto error;
> }
> - } while (time_diff_ms(&cur_time, &orig_time) < timeout);
> + if (timespec_to_ms(timespec_abs_diff(cur_time, orig_time), &diff_ms) < 0) {
> + ERR("timespec_to_ms input overflows milliseconds output");
> + errno = EOVERFLOW;
> + connect_ret = -1;
> + goto error;
> + }
> + } while (diff_ms < timeout);
>
> /* Timeout */
> errno = ETIMEDOUT;
> diff --git a/src/common/sessiond-comm/inet6.c b/src/common/sessiond-comm/inet6.c
> index dfb5fc5d..5c1ac2e0 100644
> --- a/src/common/sessiond-comm/inet6.c
> +++ b/src/common/sessiond-comm/inet6.c
> @@ -110,31 +110,13 @@ int connect_no_timeout(struct lttcomm_sock *sock)
> sizeof(sock->sockaddr.addr.sin6));
> }
>
> -/*
> - * Return time_a - time_b in milliseconds.
> - */
> -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;
> -
> - sec_diff = time_a->tv_sec - time_b->tv_sec;
> - nsec_diff = 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;
> -}
> -
> static
> int connect_with_timeout(struct lttcomm_sock *sock)
> {
> unsigned long timeout = lttcomm_get_network_timeout();
> int ret, flags, connect_ret;
> struct timespec orig_time, cur_time;
> + unsigned long diff_ms;
>
> ret = fcntl(sock->fd, F_GETFL, 0);
> if (ret == -1) {
> @@ -216,7 +198,13 @@ int connect_with_timeout(struct lttcomm_sock *sock)
> connect_ret = ret;
> goto error;
> }
> - } while (time_diff_ms(&cur_time, &orig_time) < timeout);
> + if (timespec_to_ms(timespec_abs_diff(cur_time, orig_time), &diff_ms) < 0) {
> + ERR("timespec_to_ms input overflows milliseconds output");
> + errno = EOVERFLOW;
> + connect_ret = -1;
> + goto error;
> + }
> + } while (diff_ms < timeout);
>
> /* Timeout */
> errno = ETIMEDOUT;
> diff --git a/src/common/utils.c b/src/common/utils.c
> index 3442bef8..5b4e1d3c 100644
> --- a/src/common/utils.c
> +++ b/src/common/utils.c
> @@ -31,6 +31,7 @@
> #include <pwd.h>
> #include <sys/file.h>
> #include <unistd.h>
> +#include <stdint.h>
>
> #include <common/common.h>
> #include <common/runas.h>
> @@ -41,6 +42,7 @@
>
> #include "utils.h"
> #include "defaults.h"
> +#include "time.h"
>
> /*
> * Return a partial realpath(3) of the path even if the full path does not
> @@ -1645,3 +1647,32 @@ int utils_show_help(int section, const char *page_name,
> end:
> return ret;
> }
> +
> +LTTNG_HIDDEN
> +int timespec_to_ms(struct timespec ts, unsigned long *ms)
> +{
> + unsigned long res;
> +
> + if (ts.tv_sec + 1 > ULONG_MAX / MSEC_PER_SEC) {
> + return -1;
> + }
> + res = ts.tv_sec * MSEC_PER_SEC;
> + res += ts.tv_nsec / NSEC_PER_MSEC;
> + *ms = res;
> + return 0;
> +}
> +
> +LTTNG_HIDDEN
> +struct timespec timespec_abs_diff(struct timespec t1, struct timespec t2)
> +{
> + uint64_t ts1 = (uint64_t) t1.tv_sec * (uint64_t) NSEC_PER_SEC +
> + (uint64_t) t1.tv_nsec;
> + uint64_t ts2 = (uint64_t) t2.tv_sec * (uint64_t) NSEC_PER_SEC +
> + (uint64_t) t2.tv_nsec;
> + uint64_t diff = max(ts1, ts2) - min(ts1, ts2);
> + struct timespec res;
> +
> + res.tv_sec = diff / (uint64_t) NSEC_PER_SEC;
> + res.tv_nsec = diff % (uint64_t) NSEC_PER_SEC;
> + return res;
> +}
> --
> 2.11.0
>
--
Jonathan Rajotte-Julien
EfficiOS
More information about the lttng-dev
mailing list