iproute2: prints bogus hoplimit

iproute2: prints bogus hoplimit

[Jan Engelhardt]

$ ip -6 r
fc00::/7 dev rtl0  proto kernel  metric 256  mtu 1500 advmss 1440 
hoplimit 4294967295

Most likely, "hoplimit -1" would be the right number, though I am not 
sure if simply changing %u by %d is a correct thing to do, since it 
would affect all fields.

---
 ip/iproute.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/ip/iproute.c b/ip/iproute.c
index 6a2ea05..91a8cc0 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -507,7 +507,7 @@ int print_route(const struct sockaddr_nl *who, struct nlmsghdr *n, void *arg)
 
 			if (i != RTAX_RTT && i != RTAX_RTTVAR &&
 			    i != RTAX_RTO_MIN)
-				fprintf(fp, " %u", *(unsigned*)RTA_DATA(mxrta[i]));
+				fprintf(fp, " %d", *(unsigned*)RTA_DATA(mxrta[i]));
 			else {
 				unsigned long long val = *(unsigned*)RTA_DATA(mxrta[i]);
 
-- 

Re: iproute2: prints bogus hoplimit

[Stephen Hemminger]

On Tue, 24 Feb 2009 11:27:52 +0100 (CET)
Jan Engelhardt <jengelh@medozas.de> wrote:

> 
> $ ip -6 r
> fc00::/7 dev rtl0  proto kernel  metric 256  mtu 1500 advmss 1440 
> hoplimit 4294967295
> 
> Most likely, "hoplimit -1" would be the right number, though I am not 
> sure if simply changing %u by %d is a correct thing to do, since it 
> would affect all fields.
> 
> ---
>  ip/iproute.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/ip/iproute.c b/ip/iproute.c
> index 6a2ea05..91a8cc0 100644
> --- a/ip/iproute.c
> +++ b/ip/iproute.c
> @@ -507,7 +507,7 @@ int print_route(const struct sockaddr_nl *who, struct nlmsghdr *n, void *arg)
>  
>  			if (i != RTAX_RTT && i != RTAX_RTTVAR &&
>  			    i != RTAX_RTO_MIN)
> -				fprintf(fp, " %u", *(unsigned*)RTA_DATA(mxrta[i]));
> +				fprintf(fp, " %d", *(unsigned*)RTA_DATA(mxrta[i]));
>  			else {
>  				unsigned long long val = *(unsigned*)RTA_DATA(mxrta[i]);
>  

No fix the kernel please.

Re: iproute2: prints bogus hoplimit

[Jan Engelhardt]

On Wednesday 2009-03-18 23:12, Stephen Hemminger wrote:
>On Tue, 24 Feb 2009 11:27:52 +0100 (CET)
>Jan Engelhardt <jengelh@medozas.de> wrote:
>
>> $ ip -6 r
>> fc00::/7 dev rtl0  proto kernel  metric 256  mtu 1500 advmss 1440 
>> hoplimit 4294967295
>> 
>> Most likely, "hoplimit -1" would be the right number, though I am not 
>> sure if simply changing %u by %d is a correct thing to do, since it 
>> would affect all fields.
>> 
>> ---
>>  ip/iproute.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>  	if (i != RTAX_RTT && i != RTAX_RTTVAR &&
>>  	    i != RTAX_RTO_MIN)
>> -		fprintf(fp, " %u", *(unsigned*)RTA_DATA(mxrta[i]));
>> +		fprintf(fp, " %d", *(unsigned*)RTA_DATA(mxrta[i]));
>
>No fix the kernel please.
>
And what should the kernel return?

Re: iproute2: prints bogus hoplimit

[Stephen Hemminger]

On Wed, 18 Mar 2009 23:35:12 +0100 (CET)
Jan Engelhardt <jengelh@medozas.de> wrote:

> 
> On Wednesday 2009-03-18 23:12, Stephen Hemminger wrote:
> >On Tue, 24 Feb 2009 11:27:52 +0100 (CET)
> >Jan Engelhardt <jengelh@medozas.de> wrote:
> >
> >> $ ip -6 r
> >> fc00::/7 dev rtl0  proto kernel  metric 256  mtu 1500 advmss 1440 
> >> hoplimit 4294967295
> >> 
> >> Most likely, "hoplimit -1" would be the right number, though I am not 
> >> sure if simply changing %u by %d is a correct thing to do, since it 
> >> would affect all fields.
> >> 
> >> ---
> >>  ip/iproute.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>  	if (i != RTAX_RTT && i != RTAX_RTTVAR &&
> >>  	    i != RTAX_RTO_MIN)
> >> -		fprintf(fp, " %u", *(unsigned*)RTA_DATA(mxrta[i]));
> >> +		fprintf(fp, " %d", *(unsigned*)RTA_DATA(mxrta[i]));
> >
> >No fix the kernel please.
> >
> And what should the kernel return?

The issue is that the hoplimit metric in ipv6 gets converted
to -1 (if it is zero on the incoming route request)..  

int ip6_route_add(struct fib6_config *cfg)
{
...
	  if (dst_metric(&rt->u.dst, RTAX_HOPLIMIT) == 0)
                rt->u.dst.metrics[RTAX_HOPLIMIT-1] = -1;


What should happen is when ip6 returns the route back
on the dump request (rt6_fill_node), it should translate the -1 back to 0
to keep it symmetrical and logical. The metrics are unsigned.

Re: iproute2: prints bogus hoplimit

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 18 Mar 2009 15:12:16 -0700

> On Tue, 24 Feb 2009 11:27:52 +0100 (CET)
> Jan Engelhardt <jengelh@medozas.de> wrote:
> 
> > 
> > $ ip -6 r
> > fc00::/7 dev rtl0  proto kernel  metric 256  mtu 1500 advmss 1440 
> > hoplimit 4294967295
> > 
> > Most likely, "hoplimit -1" would be the right number, though I am not 
> > sure if simply changing %u by %d is a correct thing to do, since it 
> > would affect all fields.
 ...
> > diff --git a/ip/iproute.c b/ip/iproute.c
> > index 6a2ea05..91a8cc0 100644
> > --- a/ip/iproute.c
> > +++ b/ip/iproute.c
> > @@ -507,7 +507,7 @@ int print_route(const struct sockaddr_nl *who, struct nlmsghdr *n, void *arg)
> >  
> >  			if (i != RTAX_RTT && i != RTAX_RTTVAR &&
> >  			    i != RTAX_RTO_MIN)
> > -				fprintf(fp, " %u", *(unsigned*)RTA_DATA(mxrta[i]));
> > +				fprintf(fp, " %d", *(unsigned*)RTA_DATA(mxrta[i]));
> >  			else {
> >  				unsigned long long val = *(unsigned*)RTA_DATA(mxrta[i]);
> >  
> 
> No fix the kernel please.

I don't know what you mean by this Stephen.  Why don't you take
a look at when and why the kernel uses '-1'?

"-1" is the hoplimit the kernel uses in the dst metric to mean
"use the default".

So this iproute patch is absolutely correct, or alternatively, iproute
can print "default" when it sees '-1'.

Re: iproute2: prints bogus hoplimit

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 18 Mar 2009 16:29:28 -0700

> The issue is that the hoplimit metric in ipv6 gets converted
> to -1 (if it is zero on the incoming route request)..  
> 
> int ip6_route_add(struct fib6_config *cfg)
> {
> ...
> 	  if (dst_metric(&rt->u.dst, RTAX_HOPLIMIT) == 0)
>                 rt->u.dst.metrics[RTAX_HOPLIMIT-1] = -1;
> 
> 
> What should happen is when ip6 returns the route back
> on the dump request (rt6_fill_node), it should translate the -1 back to 0
> to keep it symmetrical and logical. The metrics are unsigned.

This is how "default" is represented.

If you look, ipv4 does the same thing.

Re: iproute2: prints bogus hoplimit

[Stephen Hemminger]

On Wed, 18 Mar 2009 17:51:09 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Wed, 18 Mar 2009 15:12:16 -0700
> 
> > On Tue, 24 Feb 2009 11:27:52 +0100 (CET)
> > Jan Engelhardt <jengelh@medozas.de> wrote:
> > 
> > > 
> > > $ ip -6 r
> > > fc00::/7 dev rtl0  proto kernel  metric 256  mtu 1500 advmss 1440 
> > > hoplimit 4294967295
> > > 
> > > Most likely, "hoplimit -1" would be the right number, though I am not 
> > > sure if simply changing %u by %d is a correct thing to do, since it 
> > > would affect all fields.
>  ...
> > > diff --git a/ip/iproute.c b/ip/iproute.c
> > > index 6a2ea05..91a8cc0 100644
> > > --- a/ip/iproute.c
> > > +++ b/ip/iproute.c
> > > @@ -507,7 +507,7 @@ int print_route(const struct sockaddr_nl *who, struct nlmsghdr *n, void *arg)
> > >  
> > >  			if (i != RTAX_RTT && i != RTAX_RTTVAR &&
> > >  			    i != RTAX_RTO_MIN)
> > > -				fprintf(fp, " %u", *(unsigned*)RTA_DATA(mxrta[i]));
> > > +				fprintf(fp, " %d", *(unsigned*)RTA_DATA(mxrta[i]));
> > >  			else {
> > >  				unsigned long long val = *(unsigned*)RTA_DATA(mxrta[i]);
> > >  
> > 
> > No fix the kernel please.
> 
> I don't know what you mean by this Stephen.  Why don't you take
> a look at when and why the kernel uses '-1'?
> 
> "-1" is the hoplimit the kernel uses in the dst metric to mean
> "use the default".
> 
> So this iproute patch is absolutely correct, or alternatively, iproute
> can print "default" when it sees '-1'.
> 

if I create route with:
   ip route add ... hoplimit 0
then to do
   ip route show 
one would expect the hoplimit in the show to match the add ??
Whether the kernel fudges it in rt6_fill or utility has special case
code really doesn't matter.

The correct patch in utility would be something like:

diff --git a/ip/iproute.c b/ip/iproute.c
index 6a2ea05..bf0f31b 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -493,6 +493,8 @@ int print_route(const struct sockaddr_nl *who, struct nlmsghdr *n, void *arg)
 			mxlock = *(unsigned*)RTA_DATA(mxrta[RTAX_LOCK]);
 
 		for (i=2; i<= RTAX_MAX; i++) {
+			unsigned val;
+
 			if (mxrta[i] == NULL)
 				continue;
 			if (!hz)
@@ -505,21 +507,31 @@ int print_route(const struct sockaddr_nl *who, struct nlmsghdr *n, void *arg)
 			if (mxlock & (1<<i))
 				fprintf(fp, " lock");
 
-			if (i != RTAX_RTT && i != RTAX_RTTVAR &&
-			    i != RTAX_RTO_MIN)
-				fprintf(fp, " %u", *(unsigned*)RTA_DATA(mxrta[i]));
-			else {
-				unsigned long long val = *(unsigned*)RTA_DATA(mxrta[i]);
+			val = *(unsigned*)RTA_DATA(mxrta[i]);
+			switch (i) {
+			case RTAX_HOPLIMIT:
+				if ((long)val == -1)
+					val = 0;
+				/* fall through */
+			default:
+				fprintf(fp, " %u", val);
+				break;
 
+			case RTAX_RTT:
+			case RTAX_RTTVAR:
+			case RTAX_RTO_MIN:
 				val *= 1000;
 				if (i == RTAX_RTT)
 					val /= 8;
 				else if (i == RTAX_RTTVAR)
 					val /= 4;
+
 				if (val >= hz)
-					fprintf(fp, " %llums", val/hz);
+					fprintf(fp, " %llums",
+						(unsigned long long) val / hz);
 				else
-					fprintf(fp, " %.2fms", (float)val/hz);
+					fprintf(fp, " %.2fms", 
+						(double)val / hz);
 			}
 		}
 	}

Re: iproute2: prints bogus hoplimit

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 18 Mar 2009 18:01:06 -0700

> if I create route with:
>    ip route add ... hoplimit 0
> then to do
>    ip route show 
> one would expect the hoplimit in the show to match the add ??
> Whether the kernel fudges it in rt6_fill or utility has special case
> code really doesn't matter.

Even if the kernel should change, the tool should behave correctly
for all existing Linux kernels out there so you'd need the iproute2
hack anyways.

> The correct patch in utility would be something like:

Sure.