[lttng-dev] [PATCH lttng-tools v2] Fix: timeout for connect is not respected
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) * 100ULL) + ((A.nsec - B.nsec) / 100ULL)
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
---
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
Re: [lttng-dev] [PATCH lttng-tools v2] Fix: timeout for connect is not respected
- On Nov 9, 2018, at 10:37 AM, Jonathan Rajotte
jonathan.rajotte-jul...@efficios.com wrote:
> 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) * 100ULL) + ((A.nsec - B.nsec) / 100ULL)
>
> 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
> ---
>
> 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;
> + }
Why would a diff operation implicitly floor everything to 0 when
time_a < time_b ?
> +
> + /*
> + * Transform into MSEC, diff.tv_nsec is always positive due to borrow
> + * operation. diff.tb_sec is always positive.
tb_sec -> tv_nsec
> + */
> + return (diff.tv_sec * MSEC_PER_SEC) + (diff.tv_nsec / NSEC_PER_MSEC);
How do we handle time delta difference that overflows unsigned long ?
> }
>
> 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.
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@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
___
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
Re: [lttng-dev] [PATCH lttng-tools v2] Fix: timeout for connect is not respected
> > 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 ;).
>
> > +
> > + /*
> > +* 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?
[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@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
___
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
[lttng-dev] [PATCH lttng-tools v3] Fix: timeout for connect is not respected
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) * 100ULL) + ((A.nsec - B.nsec) / 100ULL)
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
---
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
___
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
Re: [lttng-dev] [PATCH lttng-tools v2] Fix: timeout for connect is not respected
- On Nov 9, 2018, at 1:52 PM, Jonathan Rajotte
jonathan.rajotte-jul...@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 li
[lttng-dev] [PATCH lttng-tools 2/2] Fix: max_t/min_t macros are missing cast on input
The semantic expected from max_t and min_t is to perform the max/min comparison in the type provided as first parameter. Cast the input parameters to the proper type before comparing them, rather than after. There is no more need to cast the result of the expression now that both inputs are cast to the right type. Signed-off-by: Mathieu Desnoyers --- src/common/macros.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/common/macros.h b/src/common/macros.h index c521aacd..461202ff 100644 --- a/src/common/macros.h +++ b/src/common/macros.h @@ -72,7 +72,7 @@ void *zmalloc(size_t len) #endif #ifndef max_t -#define max_t(type, a, b) ((type) max(a, b)) +#define max_t(type, a, b) max((type) a, (type) b) #endif #ifndef min @@ -80,7 +80,7 @@ void *zmalloc(size_t len) #endif #ifndef min_t -#define min_t(type, a, b) ((type) min(a, b)) +#define min_t(type, a, b) min((type) a, (type) b) #endif #ifndef LTTNG_PACKED -- 2.11.0 ___ lttng-dev mailing list lttng-dev@lists.lttng.org https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
[lttng-dev] [PATCH lttng-tools 1/2] Fix: Connect timeout arithmetic in inet/inet6
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
---
src/common/common.h | 5 +
src/common/sessiond-comm/inet.c | 28
src/common/sessiond-comm/inet6.c | 28
src/common/utils.c | 29 +
4 files changed, 50 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
+
#include "error.h"
#include "macros.h"
#include "runas.h"
#include "readwrite.h"
+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..c4759f68 100644
--- a/src/common/utils.c
+++ b/src/common/utils.c
@@ -31,6 +31,7 @@
#include
#include
#include
+#include
#include
#include
@@ -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
Re: [lttng-dev] [PATCH lttng-tools 1/2] Fix: Connect timeout arithmetic in inet/inet6
On Fri, Nov 09, 2018 at 05:27:48PM -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
> ---
> src/common/common.h | 5 +
> src/common/sessiond-comm/inet.c | 28
> src/common/sessiond-comm/inet6.c | 28
> src/common/utils.c | 29 +
> 4 files changed, 50 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
> +
> #include "error.h"
> #include "macros.h"
> #include "runas.h"
> #include "readwrite.h"
>
> +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..c4759f68 100644
> --- a/src/common/utils.c
> +++ b/src/common/ut
[lttng-dev] [PATCH lttng-tools 2/2] Fix: max_t/min_t macros are missing cast on input
The semantic expected from max_t and min_t is to perform the max/min comparison in the type provided as first parameter. Cast the input parameters to the proper type before comparing them, rather than after. There is no more need to cast the result of the expression now that both inputs are cast to the right type. Signed-off-by: Mathieu Desnoyers --- src/common/macros.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/common/macros.h b/src/common/macros.h index c521aacd..461202ff 100644 --- a/src/common/macros.h +++ b/src/common/macros.h @@ -72,7 +72,7 @@ void *zmalloc(size_t len) #endif #ifndef max_t -#define max_t(type, a, b) ((type) max(a, b)) +#define max_t(type, a, b) max((type) a, (type) b) #endif #ifndef min @@ -80,7 +80,7 @@ void *zmalloc(size_t len) #endif #ifndef min_t -#define min_t(type, a, b) ((type) min(a, b)) +#define min_t(type, a, b) min((type) a, (type) b) #endif #ifndef LTTNG_PACKED -- 2.11.0 ___ lttng-dev mailing list lttng-dev@lists.lttng.org https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
[lttng-dev] [PATCH lttng-tools 1/2] Fix: Connect timeout arithmetic in inet/inet6 (v2)
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
---
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
+
#include "error.h"
#include "macros.h"
#include "runas.h"
#include "readwrite.h"
+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
#include
#include
+#include
#include
#include
@@ -41,6 +42,7 @@
#include "utils.h"
#include "defaults.h"
+#include "time.h"
/*
* Return a partial r
Re: [lttng-dev] [PATCH lttng-tools 1/2] Fix: Connect timeout arithmetic in inet/inet6
- On Nov 9, 2018, at 5:34 PM, Jonathan Rajotte
jonathan.rajotte-jul...@efficios.com wrote:
[...]
>> +
>> +LTTNG_HIDDEN
>> +struct timespec timespec_abs_diff(struct timespec t1, struct timespec t2)
>> +{
>> +uint64_t ts1 = (uint64_t) t1.tv_sec * INT64_C(10) + (uint64_t)
>> t1.tv_nsec;
>> +uint64_t ts2 = (uint64_t) t2.tv_sec * INT64_C(10) + (uint64_t)
>> t2.tv_nsec;
>
> Use NSEC_PER_SEC here.
>
>> +uint64_t diff = max(ts1, ts2) - min(ts1, ts2);
>> +struct timespec res;
>> +
>> +res.tv_sec = diff / INT64_C(10);
>
> Use NSEC_PER_SEC here.
>
>> +res.tv_nsec = diff % INT64_C(10);
>
> Use NSEC_PER_SEC here.
Fixing in v2, thanks,
Mathieu
>
>> +return res;
>> +}
>> --
>> 2.11.0
>>
>
> --
> Jonathan Rajotte-Julien
> EfficiOS
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
___
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
Re: [lttng-dev] [PATCH lttng-tools 1/2] Fix: Connect timeout arithmetic in inet/inet6 (v2)
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 1ms
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 1ms
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
> ---
> 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
> +
> #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_
Re: [lttng-dev] [PATCH lttng-tools 1/2] Fix: Connect timeout arithmetic in inet/inet6 (v2)
> 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
> #include
> #include
> +#include
>
> #include
> #include
> @@ -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;
> + }
This is not accurate. It return -1 for the all the following valid timespec:
tv_sec = 4294967
0 < tv_nsec < 29500
You will need to check multiplication and addition separately.
> + 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
___
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
