Re: [PATCH] [IPROUTE]: A workaround to make larger rto_min printed correctly
On 21-12-2007 03:24, Satoru SATOH wrote:
2007/12/21, Jarek Poplawski [EMAIL PROTECTED]:
Jarek Poplawski wrote, On 12/20/2007 09:24 PM:
...
but since it's your patch, I hope you do some additional checking
if it's always like this...
...or maybe only changing this all a little bit will make it look safer!
Jarek P.
OK, how about this?
Signed-off-by: Satoru SATOH [EMAIL PROTECTED]
ip/iproute.c | 12
1 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/ip/iproute.c b/ip/iproute.c
index f4200ae..c771b34 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -510,16 +510,20 @@ int print_route(const struct sockaddr_nl *who,
struct nlmsghdr *n, void *arg)
fprintf(fp, %u,
*(unsigned*)RTA_DATA(mxrta[i]));
else {
unsigned val = *(unsigned*)RTA_DATA(mxrta[i]);
+ unsigned hz1 = hz;
+ if (hz1 1000)
Looks OK (safe) to me: it's compatible both with old and new way.
I'd only suggest to maybe change this to '(hz1 1024)', because
it's the biggest HZ currently in the kernel, so this compatibility
should be 100%. I think, you could leave 1 empty line before this
'if', as well. (Btw., aren't these overflows connected with
CONFIG_HIGH_RES_TIMERS?)
On the other hand this 'hz' still looks 'strange' here - I don't
understand, why, a bit earlier it's:
if (!hz)
hz = get_hz();
while 'else' would use: hz == get_user_hz();
So, probably I miss something, but even after your patch, there
could be different outputs here...
Thanks,
Jarek P.
PS: did you CC Stephen Hemminger on this?
+ hz1 /= 1000;
+ else
+ val *= 1000;
- val *= 1000;
if (i == RTAX_RTT)
val /= 8;
else if (i == RTAX_RTTVAR)
val /= 4;
- if (val = hz)
- fprintf(fp, %ums, val/hz);
+ if (val = hz1)
+ fprintf(fp, %ums, val/hz1);
else
- fprintf(fp, %.2fms, (float)val/hz);
+ fprintf(fp, %.2fms, (float)val/hz1);
}
}
}
Thanks,
Satoru SATOH
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [IPROUTE]: A workaround to make larger rto_min printed correctly
In article [EMAIL PROTECTED] (at Fri, 21 Dec 2007 11:24:54 +0900), Satoru
SATOH [EMAIL PROTECTED] says:
2007/12/21, Jarek Poplawski [EMAIL PROTECTED]:
Jarek Poplawski wrote, On 12/20/2007 09:24 PM:
...
but since it's your patch, I hope you do some additional checking
if it's always like this...
...or maybe only changing this all a little bit will make it look safer!
Jarek P.
OK, how about this?
Signed-off-by: Satoru SATOH [EMAIL PROTECTED]
ip/iproute.c | 12
1 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/ip/iproute.c b/ip/iproute.c
index f4200ae..c771b34 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -510,16 +510,20 @@ int print_route(const struct sockaddr_nl *who,
struct nlmsghdr *n, void *arg)
fprintf(fp, %u,
*(unsigned*)RTA_DATA(mxrta[i]));
else {
unsigned val = *(unsigned*)RTA_DATA(mxrta[i]);
+ unsigned hz1 = hz;
+ if (hz1 1000)
Why don't you simply use unsigned long long (or maybe uint64_t) here?
Signed-off-by: YOSHIFUJI Hideaki [EMAIL PROTECTED]
---
diff --git a/ip/iproute.c b/ip/iproute.c
index f4200ae..db9a3b6 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -509,16 +509,21 @@ int print_route(const struct sockaddr_nl *who, struct
nlmsghdr *n, void *arg)
i != RTAX_RTO_MIN)
fprintf(fp, %u,
*(unsigned*)RTA_DATA(mxrta[i]));
else {
- unsigned val = *(unsigned*)RTA_DATA(mxrta[i]);
+ unsigned long long val =
*(unsigned*)RTA_DATA(mxrta[i]);
+ unsigned div = 1;
- val *= 1000;
if (i == RTAX_RTT)
- val /= 8;
+ div = 8;
else if (i == RTAX_RTTVAR)
- val /= 4;
- if (val = hz)
- fprintf(fp, %ums, val/hz);
+ div = 4;
else
+ div = 1;
+
+ val = val * 1000ULL / div;
+
+ if (val = hz) {
+ fprintf(fp, %llums, val/hz);
+ } else
fprintf(fp, %.2fms, (float)val/hz);
}
}
--yoshfuji
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [IPROUTE]: A workaround to make larger rto_min printed correctly
On Fri, 21 Dec 2007, YOSHIFUJI Hideaki wrote:
In article [EMAIL PROTECTED] (at Fri, 21 Dec 2007 11:24:54 +0900), Satoru
SATOH [EMAIL PROTECTED] says:
2007/12/21, Jarek Poplawski [EMAIL PROTECTED]:
Jarek Poplawski wrote, On 12/20/2007 09:24 PM:
...
but since it's your patch, I hope you do some additional checking
if it's always like this...
...or maybe only changing this all a little bit will make it look safer!
Jarek P.
OK, how about this?
Signed-off-by: Satoru SATOH [EMAIL PROTECTED]
ip/iproute.c | 12
1 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/ip/iproute.c b/ip/iproute.c
index f4200ae..c771b34 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -510,16 +510,20 @@ int print_route(const struct sockaddr_nl *who,
struct nlmsghdr *n, void *arg)
fprintf(fp, %u,
*(unsigned*)RTA_DATA(mxrta[i]));
else {
unsigned val = *(unsigned*)RTA_DATA(mxrta[i]);
+ unsigned hz1 = hz;
+ if (hz1 1000)
Why don't you simply use unsigned long long (or maybe uint64_t) here?
I was wondering that too. And maybe change the (float) cast
to (double) in the fprintf.
-Bill
Signed-off-by: YOSHIFUJI Hideaki [EMAIL PROTECTED]
---
diff --git a/ip/iproute.c b/ip/iproute.c
index f4200ae..db9a3b6 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -509,16 +509,21 @@ int print_route(const struct sockaddr_nl *who, struct
nlmsghdr *n, void *arg)
i != RTAX_RTO_MIN)
fprintf(fp, %u,
*(unsigned*)RTA_DATA(mxrta[i]));
else {
- unsigned val = *(unsigned*)RTA_DATA(mxrta[i]);
+ unsigned long long val =
*(unsigned*)RTA_DATA(mxrta[i]);
+ unsigned div = 1;
- val *= 1000;
if (i == RTAX_RTT)
- val /= 8;
+ div = 8;
else if (i == RTAX_RTTVAR)
- val /= 4;
- if (val = hz)
- fprintf(fp, %ums, val/hz);
+ div = 4;
else
+ div = 1;
+
+ val = val * 1000ULL / div;
+
+ if (val = hz) {
+ fprintf(fp, %llums, val/hz);
+ } else
fprintf(fp, %.2fms, (float)val/hz);
}
}
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [IPROUTE]: A workaround to make larger rto_min printed correctly
I agree.
I mistakenly thought hz in that context must be larger than 1000..
As it's uncertain, your's looks much simpler and better.
(btw, the lines else div = 1 is not needed, is it?)
Thanks,
Satoru SATOH
2007/12/21, YOSHIFUJI Hideaki / 吉藤英明 [EMAIL PROTECTED]:
(snip)
Why don't you simply use unsigned long long (or maybe uint64_t) here?
Signed-off-by: YOSHIFUJI Hideaki [EMAIL PROTECTED]
---
diff --git a/ip/iproute.c b/ip/iproute.c
index f4200ae..db9a3b6 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -509,16 +509,21 @@ int print_route(const struct sockaddr_nl *who, struct
nlmsghdr *n, void *arg)
i != RTAX_RTO_MIN)
fprintf(fp, %u,
*(unsigned*)RTA_DATA(mxrta[i]));
else {
- unsigned val = *(unsigned*)RTA_DATA(mxrta[i]);
+ unsigned long long val =
*(unsigned*)RTA_DATA(mxrta[i]);
+ unsigned div = 1;
- val *= 1000;
if (i == RTAX_RTT)
- val /= 8;
+ div = 8;
else if (i == RTAX_RTTVAR)
- val /= 4;
- if (val = hz)
- fprintf(fp, %ums, val/hz);
+ div = 4;
else
+ div = 1;
+
+ val = val * 1000ULL / div;
+
+ if (val = hz) {
+ fprintf(fp, %llums, val/hz);
+ } else
fprintf(fp, %.2fms, (float)val/hz);
}
}
--yoshfuji
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [IPROUTE]: A workaround to make larger rto_min printed correctly
In article [EMAIL PROTECTED] (at Fri, 21 Dec 2007 22:49:59 +0900), Satoru
SATOH [EMAIL PROTECTED] says:
I agree.
I mistakenly thought hz in that context must be larger than 1000..
As it's uncertain, your's looks much simpler and better.
(btw, the lines else div = 1 is not needed, is it?)
Simplest fix is as follows:
Signed-off-by: YOSHIFUJI Hideaki [EMAIL PROTECTED]
--
diff --git a/ip/iproute.c b/ip/iproute.c
index f4200ae..7a885b0 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -509,7 +509,7 @@ int print_route(const struct sockaddr_nl *who, struct
nlmsghdr *n, void *arg)
i != RTAX_RTO_MIN)
fprintf(fp, %u,
*(unsigned*)RTA_DATA(mxrta[i]));
else {
- unsigned val = *(unsigned*)RTA_DATA(mxrta[i]);
+ unsigned long long val =
*(unsigned*)RTA_DATA(mxrta[i]);
val *= 1000;
if (i == RTAX_RTT)
@@ -517,7 +517,7 @@ int print_route(const struct sockaddr_nl *who, struct
nlmsghdr *n, void *arg)
else if (i == RTAX_RTTVAR)
val /= 4;
if (val = hz)
- fprintf(fp, %ums, val/hz);
+ fprintf(fp, %llums, val/hz);
else
fprintf(fp, %.2fms, (float)val/hz);
}
--
YOSHIFUJI Hideaki @ USAGI Project [EMAIL PROTECTED]
GPG-FP : 9022 65EB 1ECF 3AD1 0BDF 80D8 4807 F894 E062 0EEA
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [IPROUTE]: A workaround to make larger rto_min printed correctly
On Fri, 21 Dec 2007 22:58:04 +0900 (JST)
YOSHIFUJI Hideaki / 吉藤英明 [EMAIL PROTECTED] wrote:
In article [EMAIL PROTECTED] (at Fri, 21 Dec 2007 22:49:59 +0900), Satoru
SATOH [EMAIL PROTECTED] says:
I agree.
I mistakenly thought hz in that context must be larger than 1000..
As it's uncertain, your's looks much simpler and better.
(btw, the lines else div = 1 is not needed, is it?)
Simplest fix is as follows:
Signed-off-by: YOSHIFUJI Hideaki [EMAIL PROTECTED]
--
diff --git a/ip/iproute.c b/ip/iproute.c
index f4200ae..7a885b0 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -509,7 +509,7 @@ int print_route(const struct sockaddr_nl *who, struct
nlmsghdr *n, void *arg)
i != RTAX_RTO_MIN)
fprintf(fp, %u,
*(unsigned*)RTA_DATA(mxrta[i]));
else {
- unsigned val = *(unsigned*)RTA_DATA(mxrta[i]);
+ unsigned long long val =
*(unsigned*)RTA_DATA(mxrta[i]);
val *= 1000;
if (i == RTAX_RTT)
@@ -517,7 +517,7 @@ int print_route(const struct sockaddr_nl *who, struct
nlmsghdr *n, void *arg)
else if (i == RTAX_RTTVAR)
val /= 4;
if (val = hz)
- fprintf(fp, %ums, val/hz);
+ fprintf(fp, %llums, val/hz);
else
fprintf(fp, %.2fms, (float)val/hz);
}
applied thanks.
--
Stephen Hemminger [EMAIL PROTECTED]
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [IPROUTE]: A workaround to make larger rto_min printed correctly
In article [EMAIL PROTECTED] (at Thu, 20 Dec 2007 12:31:27 +0900), Satoru
SATOH [EMAIL PROTECTED] says:
diff --git a/ip/iproute.c b/ip/iproute.c
index f4200ae..fa722c6 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -510,16 +510,16 @@ int print_route(const struct sockaddr_nl *who,
struct nlmsghdr *n, void *arg)
fprintf(fp, %u,
*(unsigned*)RTA_DATA(mxrta[i]));
else {
unsigned val = *(unsigned*)RTA_DATA(mxrta[i]);
+ unsigned hz1 = hz / 1000;
- val *= 1000;
if (i == RTAX_RTT)
I think this is incorrect; hz might not be 1000; e.g. 250 etc.
--yoshfuji
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [IPROUTE]: A workaround to make larger rto_min printed correctly
On 20-12-2007 04:31, Satoru SATOH wrote:
ip route show does not print correct value when larger rto_min is
set (e.g. 3sec).
This problem is because of overflow in print_route() and
the patch below is a workaround fix for that.
...
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -510,16 +510,16 @@ int print_route(const struct sockaddr_nl *who,
struct nlmsghdr *n, void *arg)
fprintf(fp, %u,
*(unsigned*)RTA_DATA(mxrta[i]));
else {
unsigned val = *(unsigned*)RTA_DATA(mxrta[i]);
+ unsigned hz1 = hz / 1000;
...
+ if (val = hz1)
+ fprintf(fp, %ums, val/hz1);
...
Probably I miss something or my iproute sources are too old, but:
does this work with hz 1000?
Regards,
Jarek P.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [IPROUTE]: A workaround to make larger rto_min printed correctly
i see. HZ can be 1000.. i should be wrong.
however, i got the following,
[root iproute2.org]# ./ip/ip route change 192.168.140.0/24 dev eth1 rto_min 4s
[root iproute2.org]# gdb -q ./ip/ip
Using host libthread_db library /lib/libthread_db.so.1.
(gdb) br iproute.c:512
Breakpoint 1 at 0x804fc8d: file iproute.c, line 512.
(gdb) r route show dev eth1
Starting program: /root/iproute2.org/ip/ip route show dev eth1
Breakpoint 1, print_route (who=0xbfb9854c, n=0xbfb94528, arg=0x6404c0)
at iproute.c:512
512 unsigned val =
*(unsigned*)RTA_DATA(mxrta[i]);
(gdb) l 512,522
512 unsigned val =
*(unsigned*)RTA_DATA(mxrta[i]);
513
514 val *= 1000;
515 if (i == RTAX_RTT)
516 val /= 8;
517 else if (i == RTAX_RTTVAR)
518 val /= 4;
519 if (val = hz)
520 fprintf(fp, %ums, val/hz);
521 else
522 fprintf(fp, %.2fms,
(float)val/hz);
(gdb) p hz
$1 = 10
(gdb) n
514 val *= 1000;
(gdb) p val
$2 = 40
(gdb) p val/ (hz / 1000)
$3 = 4000
(gdb) n
515 if (i == RTAX_RTT)
(gdb) p val
$4 = 1385447424
(gdb) c
Continuing.
192.168.140.0/24 scope link rto_min lock 1ms
Program exited normally.
(gdb)
Thanks,
Satoru SATOH
2007/12/20, Jarek Poplawski [EMAIL PROTECTED]:
On 20-12-2007 04:31, Satoru SATOH wrote:
ip route show does not print correct value when larger rto_min is
set (e.g. 3sec).
This problem is because of overflow in print_route() and
the patch below is a workaround fix for that.
...
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -510,16 +510,16 @@ int print_route(const struct sockaddr_nl *who,
struct nlmsghdr *n, void *arg)
fprintf(fp, %u,
*(unsigned*)RTA_DATA(mxrta[i]));
else {
unsigned val =
*(unsigned*)RTA_DATA(mxrta[i]);
+ unsigned hz1 = hz / 1000;
...
+ if (val = hz1)
+ fprintf(fp, %ums, val/hz1);
...
Probably I miss something or my iproute sources are too old, but:
does this work with hz 1000?
Regards,
Jarek P.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [IPROUTE]: A workaround to make larger rto_min printed correctly
Satoru SATOH wrote, On 12/20/2007 05:21 PM: i see. HZ can be 1000.. i should be wrong. however, i got the following, [root iproute2.org]# ./ip/ip route change 192.168.140.0/24 dev eth1 rto_min 4s [root iproute2.org]# gdb -q ./ip/ip ... (gdb) p hz $1 = 10 That's why I had some doubts! I didn't study this enough, but my (older) version definitely showed hz == 100. Maybe I'm wrong, but looking into lib/util.c it seems this could be set differently depending on system's configuration (or even kernel version). So, probably this patch could sometimes work even for HZ 1000, but since it's your patch, I hope you do some additional checking if it's always like this... Cheers, Jarek P. -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [IPROUTE]: A workaround to make larger rto_min printed correctly
Jarek Poplawski wrote, On 12/20/2007 09:24 PM: ... but since it's your patch, I hope you do some additional checking if it's always like this... ...or maybe only changing this all a little bit will make it look safer! Jarek P. -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [IPROUTE]: A workaround to make larger rto_min printed correctly
2007/12/21, Jarek Poplawski [EMAIL PROTECTED]:
Jarek Poplawski wrote, On 12/20/2007 09:24 PM:
...
but since it's your patch, I hope you do some additional checking
if it's always like this...
...or maybe only changing this all a little bit will make it look safer!
Jarek P.
OK, how about this?
Signed-off-by: Satoru SATOH [EMAIL PROTECTED]
ip/iproute.c | 12
1 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/ip/iproute.c b/ip/iproute.c
index f4200ae..c771b34 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -510,16 +510,20 @@ int print_route(const struct sockaddr_nl *who,
struct nlmsghdr *n, void *arg)
fprintf(fp, %u,
*(unsigned*)RTA_DATA(mxrta[i]));
else {
unsigned val = *(unsigned*)RTA_DATA(mxrta[i]);
+ unsigned hz1 = hz;
+ if (hz1 1000)
+ hz1 /= 1000;
+ else
+ val *= 1000;
- val *= 1000;
if (i == RTAX_RTT)
val /= 8;
else if (i == RTAX_RTTVAR)
val /= 4;
- if (val = hz)
- fprintf(fp, %ums, val/hz);
+ if (val = hz1)
+ fprintf(fp, %ums, val/hz1);
else
- fprintf(fp, %.2fms, (float)val/hz);
+ fprintf(fp, %.2fms, (float)val/hz1);
}
}
}
Thanks,
Satoru SATOH
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] [IPROUTE]: A workaround to make larger rto_min printed correctly
ip route show does not print correct value when larger rto_min is
set (e.g. 3sec).
This problem is because of overflow in print_route() and
the patch below is a workaround fix for that.
[root test]# ./iproute2.git.org/ip/ip route show dev eth1
192.168.140.0/24 proto kernel scope link src 192.168.140.130
169.254.0.0/16 scope link
[root test]# ./iproute2.git.org/ip/ip route change 192.168.140.0/24
dev eth1 rto_min 3s
[root test]# ./iproute2.git.org/ip/ip route show dev eth1
192.168.140.0/24 scope link rto_min lock 2ms -- wrong
169.254.0.0/16 scope link
[root test]# ./iproute2.git/ip/ip route show dev eth1 # patched version
192.168.140.0/24 scope link rto_min lock 3000ms -- correct
169.254.0.0/16 scope link
[root test]#
Signed-off-by: Satoru SATOH [EMAIL PROTECTED]
ip/iproute.c |8
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/ip/iproute.c b/ip/iproute.c
index f4200ae..fa722c6 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -510,16 +510,16 @@ int print_route(const struct sockaddr_nl *who,
struct nlmsghdr *n, void *arg)
fprintf(fp, %u,
*(unsigned*)RTA_DATA(mxrta[i]));
else {
unsigned val = *(unsigned*)RTA_DATA(mxrta[i]);
+ unsigned hz1 = hz / 1000;
- val *= 1000;
if (i == RTAX_RTT)
val /= 8;
else if (i == RTAX_RTTVAR)
val /= 4;
- if (val = hz)
- fprintf(fp, %ums, val/hz);
+ if (val = hz1)
+ fprintf(fp, %ums, val/hz1);
else
- fprintf(fp, %.2fms, (float)val/hz);
+ fprintf(fp, %.2fms, (float)val/hz1);
}
}
}
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
