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

[Mathieu Desnoyers]

----- On Nov 9, 2018, at 1:52 PM, Jonathan Rajotte jonathan.rajotte-julien at efficios.com wrote:

>> > 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;
>> > +	}
>> 
>> Why would a diff operation implicitly floor everything to 0 when
>> time_a < time_b ?
> 
> Sure I can implement it as a real difference without the same "assumptions" of
> time_a > time_b since we are comparing passing time.
> 
> Make sure to tell this to past Mathieu ;).

I'll try, as soon as I find a time machine :)

> 
>> 
>> > +
>> > +	/*
>> > +	 * Transform into MSEC, diff.tv_nsec is always positive due to borrow
>> > +	 * operation. diff.tb_sec is always positive.
>> 
>> tb_sec -> tv_nsec
> 
> Fixed.
> 
>> 
>> 
>> > +	 */
>> > +	return (diff.tv_sec * MSEC_PER_SEC) + (diff.tv_nsec / NSEC_PER_MSEC);
>> 
>> How do we handle time delta difference that overflows unsigned long ?
> 
> Let's do a quick calculation. Correct me if anything is wrong here.
> 
> How many msec does a unsigned long hold?
> A long is *minimum* 32 bits. Yielding a maximum value of 4294967295. [1]
> 
> Lets assume that the nanoseconds part of the diff struct is maxed out. The
> nanoseconds part yield 99 ms.
> 
> 4294967295 - 99 = 4294967196ms
> 
> How big would the second field of diff be a problem?
> 
> 4294967196/ MSEC_PER_SEC = 4294967s or 49 days.
> 
> I would err on the side of "that it's enough". We also have to take into
> account that for this to happen either something is going horribly wrong during
> getting the current time or the user purposely set the timeout to this value
> since we sample every 200ms.
> Which at this point goes into the PEBKAC category.
> 
> Do you agree?

It would be cleaner to return the result in an output parameter, return 0 on success,
negative error on error, and check for errors in the caller.

Thus, we can figure out if the result end up overflowing unsigned long, and return
an error accordingly.

Thanks,

Mathieu


> 
> [1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf p.22 §5.2.4.2.1
> 
>> 
>> > }
>> > 
>> > 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)
>> 
>> This should be moved to a common implementation file. It's a cut and
>> paste from inet.c.
> 
> Past Mathieu did not seems to bother here.
> 
> I'm open into doing this in two patches since this should be backported. Do you
> have a suggestion regarding where to put it?
> 
>> 
>> Thanks,
>> 
>> Mathieu
>> 
>> > {
>> > -	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 at lists.lttng.org
>> > https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>> 
>> --
>> Mathieu Desnoyers
>> EfficiOS Inc.
>> http://www.efficios.com
> 
> --
> Jonathan Rajotte-Julien
> EfficiOS

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