| Message ID | 1460531032-17653-1-git-send-email-nikunj@linux.vnet.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
On 13.04.2016 09:03, Nikunj A Dadhania wrote: > ping was failing for machine across the subnet with > statically assinged IP address. The parsed gateway address > was ignored in the stack because the router variable was not > set. > > Currently, ping does not take a netmask parameter, set netmask parameter > with the common top order bits of client IP and gateway IP. > > Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> > --- > clients/net-snk/app/netapps/ping.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/clients/net-snk/app/netapps/ping.c b/clients/net-snk/app/netapps/ping.c > index 2c7dadb..1522792 100644 > --- a/clients/net-snk/app/netapps/ping.c > +++ b/clients/net-snk/app/netapps/ping.c > @@ -163,6 +163,12 @@ ping(int argc, char *argv[]) > > } else { > memcpy(&fn_ip.own_ip, &ping_args.client_ip.integer, 4); > + if (ping_args.gateway_ip.integer) { > + uint32_t netmask; > + netmask = ping_args.gateway_ip.integer & ping_args.client_ip.integer; > + set_ipv4_netmask(netmask & 0xFFFFFF00); That netmask calculation looks somewhat strange to me ... what's the rationale here? If you need automatic netmask calculation, shouldn't that rather be done by the type of IP address that has been specified? (or maybe via an "/xx" suffix of the specified IP address string?) Thomas > + set_ipv4_router(ping_args.gateway_ip.integer); > + } > arp_failed = 1; > printf(" Own IP address: "); > } >
Thomas Huth <thuth@redhat.com> writes: > On 13.04.2016 09:03, Nikunj A Dadhania wrote: >> ping was failing for machine across the subnet with >> statically assinged IP address. The parsed gateway address >> was ignored in the stack because the router variable was not >> set. >> >> Currently, ping does not take a netmask parameter, set netmask parameter >> with the common top order bits of client IP and gateway IP. >> >> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >> --- >> clients/net-snk/app/netapps/ping.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/clients/net-snk/app/netapps/ping.c b/clients/net-snk/app/netapps/ping.c >> index 2c7dadb..1522792 100644 >> --- a/clients/net-snk/app/netapps/ping.c >> +++ b/clients/net-snk/app/netapps/ping.c >> @@ -163,6 +163,12 @@ ping(int argc, char *argv[]) >> >> } else { >> memcpy(&fn_ip.own_ip, &ping_args.client_ip.integer, 4); >> + if (ping_args.gateway_ip.integer) { >> + uint32_t netmask; >> + netmask = ping_args.gateway_ip.integer & ping_args.client_ip.integer; >> + set_ipv4_netmask(netmask & 0xFFFFFF00); > > That netmask calculation looks somewhat strange to me ... what's the > rationale here? Ping currently only supports IPv4. Case here was that subnet mask is not provided and we are trying to guess the subnet mask. Dont think this will be accurate always, but will work. > If you need automatic netmask calculation, shouldn't > that rather be done by the type of IP address that has been specified? Here its only IPv4. We still dont have ipv6 support here. I have that in my list though. > (or maybe via an "/xx" suffix of the specified IP address string?) I am not sure about this, can you please elaborate? Regards Nikunj
On 22.04.2016 07:23, Nikunj A Dadhania wrote: > Thomas Huth <thuth@redhat.com> writes: > >> On 13.04.2016 09:03, Nikunj A Dadhania wrote: >>> ping was failing for machine across the subnet with >>> statically assinged IP address. The parsed gateway address >>> was ignored in the stack because the router variable was not >>> set. >>> >>> Currently, ping does not take a netmask parameter, set netmask parameter >>> with the common top order bits of client IP and gateway IP. >>> >>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>> --- >>> clients/net-snk/app/netapps/ping.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/clients/net-snk/app/netapps/ping.c b/clients/net-snk/app/netapps/ping.c >>> index 2c7dadb..1522792 100644 >>> --- a/clients/net-snk/app/netapps/ping.c >>> +++ b/clients/net-snk/app/netapps/ping.c >>> @@ -163,6 +163,12 @@ ping(int argc, char *argv[]) >>> >>> } else { >>> memcpy(&fn_ip.own_ip, &ping_args.client_ip.integer, 4); >>> + if (ping_args.gateway_ip.integer) { >>> + uint32_t netmask; >>> + netmask = ping_args.gateway_ip.integer & ping_args.client_ip.integer; >>> + set_ipv4_netmask(netmask & 0xFFFFFF00); >> >> That netmask calculation looks somewhat strange to me ... what's the >> rationale here? > > Ping currently only supports IPv4. Case here was that subnet mask is not > provided and we are trying to guess the subnet mask. Dont think this > will be accurate always, but will work. I think this will be very fragile. For example, let's assume client address is 192.168.1.10, router address is 192.168.1.1 and TFTP server address is 192.168.3.2 (i.e. TFTP server is in another subnet). With your calculation, you get netmask = 192.168.1.0. When the send_ipv4() code then tries to find out whether the TFTP server is in the same network as the client (to find out whether a router MAC has to be used), it does: if((subnet_mask & own_ip) == (subnet_mask & ip->ip_dst)) ... and thus in above example, it will think that the TFTP server is in the same subnet as the client and fail to use the router address. >> If you need automatic netmask calculation, shouldn't >> that rather be done by the type of IP address that has been specified? > > Here its only IPv4. We still dont have ipv6 support here. I have that in > my list though. I didn't mean IPv6, I was talking about IPv4 address classes (sorry, should have used that term). For Class A, B and C, the default netmask should be applied as long as no CIDR prefix length has been specified (see the first table on https://en.wikipedia.org/wiki/IPv4_subnetting_reference) >> (or maybe via an "/xx" suffix of the specified IP address string?) > > I am not sure about this, can you please elaborate? Sorry again for not using the right terms ... I meant the specifying the prefix length (and thus the netmask) with a CIDR prefix notation, see https://en.wikipedia.org/wiki/Classless_Inter-Domain_Routing#CIDR_notation That means, it might maybe be useful to support specifying the client address with a prefix length like this: load net:192.168.3.2,filename,192.168.1.10/24 What do you think? Thomas
Thomas Huth <thuth@redhat.com> writes: > On 22.04.2016 07:23, Nikunj A Dadhania wrote: >> Thomas Huth <thuth@redhat.com> writes: >> >>> On 13.04.2016 09:03, Nikunj A Dadhania wrote: >>>> ping was failing for machine across the subnet with >>>> statically assinged IP address. The parsed gateway address >>>> was ignored in the stack because the router variable was not >>>> set. >>>> >>>> Currently, ping does not take a netmask parameter, set netmask parameter >>>> with the common top order bits of client IP and gateway IP. >>>> >>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>>> --- >>>> clients/net-snk/app/netapps/ping.c | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/clients/net-snk/app/netapps/ping.c b/clients/net-snk/app/netapps/ping.c >>>> index 2c7dadb..1522792 100644 >>>> --- a/clients/net-snk/app/netapps/ping.c >>>> +++ b/clients/net-snk/app/netapps/ping.c >>>> @@ -163,6 +163,12 @@ ping(int argc, char *argv[]) >>>> >>>> } else { >>>> memcpy(&fn_ip.own_ip, &ping_args.client_ip.integer, 4); >>>> + if (ping_args.gateway_ip.integer) { >>>> + uint32_t netmask; >>>> + netmask = ping_args.gateway_ip.integer & ping_args.client_ip.integer; >>>> + set_ipv4_netmask(netmask & 0xFFFFFF00); >>> >>> That netmask calculation looks somewhat strange to me ... what's the >>> rationale here? >> >> Ping currently only supports IPv4. Case here was that subnet mask is not >> provided and we are trying to guess the subnet mask. Dont think this >> will be accurate always, but will work. > > I think this will be very fragile. For example, let's assume client > address is 192.168.1.10, router address is 192.168.1.1 and TFTP server > address is 192.168.3.2 (i.e. TFTP server is in another subnet). > > With your calculation, you get netmask = 192.168.1.0. Oops, yes, i should have got it as 255.255.255.0 > When the > send_ipv4() code then tries to find out whether the TFTP server is in > the same network as the client (to find out whether a router MAC has to > be used), it does: > > if((subnet_mask & own_ip) == (subnet_mask & ip->ip_dst)) ... > > and thus in above example, it will think that the TFTP server is in the > same subnet as the client and fail to use the router address. > >>> If you need automatic netmask calculation, shouldn't >>> that rather be done by the type of IP address that has been specified? >> >> Here its only IPv4. We still dont have ipv6 support here. I have that in >> my list though. > > I didn't mean IPv6, I was talking about IPv4 address classes (sorry, > should have used that term). For Class A, B and C, the default netmask > should be applied as long as no CIDR prefix length has been specified > (see the first table on > https://en.wikipedia.org/wiki/IPv4_subnetting_reference) > >>> (or maybe via an "/xx" suffix of the specified IP address string?) >> >> I am not sure about this, can you please elaborate? > > Sorry again for not using the right terms ... I meant the specifying the > prefix length (and thus the netmask) with a CIDR prefix notation, see > https://en.wikipedia.org/wiki/Classless_Inter-Domain_Routing#CIDR_notation > > That means, it might maybe be useful to support specifying the client > address with a prefix length like this: > > load net:192.168.3.2,filename,192.168.1.10/24 > > What do you think? So that will be client-ip/nn, we need to take care we dont break old cases. how about Currently, its this [bootp,ipv6,dhcp,]siaddr,filename,ciaddr,giaddr,bootp-retries,tftp-retries Change that to: [bootp,ipv6,dhcp,]siaddr,filename,ciaddr,giaddr,bootp-retries,tftp-retries,netmask One more argument at the end. Regards Nikunj
On 22.04.2016 09:39, Nikunj A Dadhania wrote: > Thomas Huth <thuth@redhat.com> writes: ... >> Sorry again for not using the right terms ... I meant the specifying the >> prefix length (and thus the netmask) with a CIDR prefix notation, see >> https://en.wikipedia.org/wiki/Classless_Inter-Domain_Routing#CIDR_notation >> >> That means, it might maybe be useful to support specifying the client >> address with a prefix length like this: >> >> load net:192.168.3.2,filename,192.168.1.10/24 >> >> What do you think? > > So that will be client-ip/nn, we need to take care we dont break old > cases. how about > > Currently, its this > > [bootp,ipv6,dhcp,]siaddr,filename,ciaddr,giaddr,bootp-retries,tftp-retries > > Change that to: > > [bootp,ipv6,dhcp,]siaddr,filename,ciaddr,giaddr,bootp-retries,tftp-retries,netmask > > One more argument at the end. That would of course work, too. But actually, I already always have a hard time to remember the current order of the arguments here ... adding some more does not sound too appealing to me. There is one more thing to consider: IPv6. Currently, the IPv6 code silently assumes a prefix length of 64 for all addresses (I think), which works in most cases, but is technically also wrong, of course. So we might need a way to specify the prefix length / netmask for IPv6, too, one day. And for IPv6, you certainly don't want to type the whole netmask but use the /prefixlen notation instead. So I'd recommend to use that for IPv4 here, too, to keep both as close as possible. Thomas
Thomas Huth <thuth@redhat.com> writes: > On 22.04.2016 09:39, Nikunj A Dadhania wrote: >> Thomas Huth <thuth@redhat.com> writes: > ... >>> Sorry again for not using the right terms ... I meant the specifying the >>> prefix length (and thus the netmask) with a CIDR prefix notation, see >>> https://en.wikipedia.org/wiki/Classless_Inter-Domain_Routing#CIDR_notation >>> >>> That means, it might maybe be useful to support specifying the client >>> address with a prefix length like this: >>> >>> load net:192.168.3.2,filename,192.168.1.10/24 >>> >>> What do you think? >> >> So that will be client-ip/nn, we need to take care we dont break old >> cases. how about >> >> Currently, its this >> >> [bootp,ipv6,dhcp,]siaddr,filename,ciaddr,giaddr,bootp-retries,tftp-retries >> >> Change that to: >> >> [bootp,ipv6,dhcp,]siaddr,filename,ciaddr,giaddr,bootp-retries,tftp-retries,netmask >> >> One more argument at the end. > > That would of course work, too. But actually, I already always have a > hard time to remember the current order of the arguments here ... adding > some more does not sound too appealing to me. > > There is one more thing to consider: IPv6. Currently, the IPv6 code > silently assumes a prefix length of 64 for all addresses (I think), > which works in most cases, but is technically also wrong, of course. So > we might need a way to specify the prefix length / netmask for IPv6, > too, one day. And for IPv6, you certainly don't want to type the whole > netmask but use the /prefixlen notation instead. So I'd recommend to use > that for IPv4 here, too, to keep both as close as possible. If we could handle this without breaking the current parsing logic, that should be fine. Regards Nikunj
On 04/22/2016 05:39 PM, Nikunj A Dadhania wrote: > Thomas Huth <thuth@redhat.com> writes: > >> On 22.04.2016 07:23, Nikunj A Dadhania wrote: >>> Thomas Huth <thuth@redhat.com> writes: >>> >>>> On 13.04.2016 09:03, Nikunj A Dadhania wrote: >>>>> ping was failing for machine across the subnet with >>>>> statically assinged IP address. The parsed gateway address >>>>> was ignored in the stack because the router variable was not >>>>> set. >>>>> >>>>> Currently, ping does not take a netmask parameter, set netmask parameter >>>>> with the common top order bits of client IP and gateway IP. >>>>> >>>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>>>> --- >>>>> clients/net-snk/app/netapps/ping.c | 6 ++++++ >>>>> 1 file changed, 6 insertions(+) >>>>> >>>>> diff --git a/clients/net-snk/app/netapps/ping.c b/clients/net-snk/app/netapps/ping.c >>>>> index 2c7dadb..1522792 100644 >>>>> --- a/clients/net-snk/app/netapps/ping.c >>>>> +++ b/clients/net-snk/app/netapps/ping.c >>>>> @@ -163,6 +163,12 @@ ping(int argc, char *argv[]) >>>>> >>>>> } else { >>>>> memcpy(&fn_ip.own_ip, &ping_args.client_ip.integer, 4); >>>>> + if (ping_args.gateway_ip.integer) { >>>>> + uint32_t netmask; >>>>> + netmask = ping_args.gateway_ip.integer & ping_args.client_ip.integer; >>>>> + set_ipv4_netmask(netmask & 0xFFFFFF00); >>>> >>>> That netmask calculation looks somewhat strange to me ... what's the >>>> rationale here? >>> >>> Ping currently only supports IPv4. Case here was that subnet mask is not >>> provided and we are trying to guess the subnet mask. Dont think this >>> will be accurate always, but will work. >> >> I think this will be very fragile. For example, let's assume client >> address is 192.168.1.10, router address is 192.168.1.1 and TFTP server >> address is 192.168.3.2 (i.e. TFTP server is in another subnet). >> >> With your calculation, you get netmask = 192.168.1.0. > > Oops, yes, i should have got it as 255.255.255.0 Does this mean that v2 is coming? > >> When the >> send_ipv4() code then tries to find out whether the TFTP server is in >> the same network as the client (to find out whether a router MAC has to >> be used), it does: >> >> if((subnet_mask & own_ip) == (subnet_mask & ip->ip_dst)) ... >> >> and thus in above example, it will think that the TFTP server is in the >> same subnet as the client and fail to use the router address. >> >>>> If you need automatic netmask calculation, shouldn't >>>> that rather be done by the type of IP address that has been specified? >>> >>> Here its only IPv4. We still dont have ipv6 support here. I have that in >>> my list though. >> >> I didn't mean IPv6, I was talking about IPv4 address classes (sorry, >> should have used that term). For Class A, B and C, the default netmask >> should be applied as long as no CIDR prefix length has been specified >> (see the first table on >> https://en.wikipedia.org/wiki/IPv4_subnetting_reference) >> >>>> (or maybe via an "/xx" suffix of the specified IP address string?) >>> >>> I am not sure about this, can you please elaborate? >> >> Sorry again for not using the right terms ... I meant the specifying the >> prefix length (and thus the netmask) with a CIDR prefix notation, see >> https://en.wikipedia.org/wiki/Classless_Inter-Domain_Routing#CIDR_notation >> >> That means, it might maybe be useful to support specifying the client >> address with a prefix length like this: >> >> load net:192.168.3.2,filename,192.168.1.10/24 >> >> What do you think? > > So that will be client-ip/nn, we need to take care we dont break old > cases. how about > > Currently, its this > > [bootp,ipv6,dhcp,]siaddr,filename,ciaddr,giaddr,bootp-retries,tftp-retries > > Change that to: > > [bootp,ipv6,dhcp,]siaddr,filename,ciaddr,giaddr,bootp-retries,tftp-retries,netmask > > > One more argument at the end. I like /nn better. Can be /255.255.255.0 as well, I cannot see how it can break compatibility really.
Alexey Kardashevskiy <aik@ozlabs.ru> writes: > On 04/22/2016 05:39 PM, Nikunj A Dadhania wrote: >> Thomas Huth <thuth@redhat.com> writes: >> >>> On 22.04.2016 07:23, Nikunj A Dadhania wrote: >>>> Thomas Huth <thuth@redhat.com> writes: >>>> >>>>> On 13.04.2016 09:03, Nikunj A Dadhania wrote: >>>>>> ping was failing for machine across the subnet with >>>>>> statically assinged IP address. The parsed gateway address >>>>>> was ignored in the stack because the router variable was not >>>>>> set. >>>>>> >>>>>> Currently, ping does not take a netmask parameter, set netmask parameter >>>>>> with the common top order bits of client IP and gateway IP. >>>>>> >>>>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>>>>> --- >>>>>> clients/net-snk/app/netapps/ping.c | 6 ++++++ >>>>>> 1 file changed, 6 insertions(+) >>>>>> >>>>>> diff --git a/clients/net-snk/app/netapps/ping.c b/clients/net-snk/app/netapps/ping.c >>>>>> index 2c7dadb..1522792 100644 >>>>>> --- a/clients/net-snk/app/netapps/ping.c >>>>>> +++ b/clients/net-snk/app/netapps/ping.c >>>>>> @@ -163,6 +163,12 @@ ping(int argc, char *argv[]) >>>>>> >>>>>> } else { >>>>>> memcpy(&fn_ip.own_ip, &ping_args.client_ip.integer, 4); >>>>>> + if (ping_args.gateway_ip.integer) { >>>>>> + uint32_t netmask; >>>>>> + netmask = ping_args.gateway_ip.integer & ping_args.client_ip.integer; >>>>>> + set_ipv4_netmask(netmask & 0xFFFFFF00); >>>>> >>>>> That netmask calculation looks somewhat strange to me ... what's the >>>>> rationale here? >>>> >>>> Ping currently only supports IPv4. Case here was that subnet mask is not >>>> provided and we are trying to guess the subnet mask. Dont think this >>>> will be accurate always, but will work. >>> >>> I think this will be very fragile. For example, let's assume client >>> address is 192.168.1.10, router address is 192.168.1.1 and TFTP server >>> address is 192.168.3.2 (i.e. TFTP server is in another subnet). >>> >>> With your calculation, you get netmask = 192.168.1.0. >> >> Oops, yes, i should have got it as 255.255.255.0 > > Does this mean that v2 is coming? Yes, i had been busy tracking the xhci keyboard issue. >> >>> When the >>> send_ipv4() code then tries to find out whether the TFTP server is in >>> the same network as the client (to find out whether a router MAC has to >>> be used), it does: >>> >>> if((subnet_mask & own_ip) == (subnet_mask & ip->ip_dst)) ... >>> >>> and thus in above example, it will think that the TFTP server is in the >>> same subnet as the client and fail to use the router address. >>> >>>>> If you need automatic netmask calculation, shouldn't >>>>> that rather be done by the type of IP address that has been specified? >>>> >>>> Here its only IPv4. We still dont have ipv6 support here. I have that in >>>> my list though. >>> >>> I didn't mean IPv6, I was talking about IPv4 address classes (sorry, >>> should have used that term). For Class A, B and C, the default netmask >>> should be applied as long as no CIDR prefix length has been specified >>> (see the first table on >>> https://en.wikipedia.org/wiki/IPv4_subnetting_reference) >>> >>>>> (or maybe via an "/xx" suffix of the specified IP address string?) >>>> >>>> I am not sure about this, can you please elaborate? >>> >>> Sorry again for not using the right terms ... I meant the specifying the >>> prefix length (and thus the netmask) with a CIDR prefix notation, see >>> https://en.wikipedia.org/wiki/Classless_Inter-Domain_Routing#CIDR_notation >>> >>> That means, it might maybe be useful to support specifying the client >>> address with a prefix length like this: >>> >>> load net:192.168.3.2,filename,192.168.1.10/24 >>> >>> What do you think? >> >> So that will be client-ip/nn, we need to take care we dont break old >> cases. how about >> >> Currently, its this >> >> [bootp,ipv6,dhcp,]siaddr,filename,ciaddr,giaddr,bootp-retries,tftp-retries >> >> Change that to: >> >> [bootp,ipv6,dhcp,]siaddr,filename,ciaddr,giaddr,bootp-retries,tftp-retries,netmask >> >> >> One more argument at the end. > > > I like /nn better. Can be /255.255.255.0 as well, I cannot see how it can > break compatibility really. Sure will give this a try. Validation of user input is sometimes tideous. Regards Nikunj
diff --git a/clients/net-snk/app/netapps/ping.c b/clients/net-snk/app/netapps/ping.c index 2c7dadb..1522792 100644 --- a/clients/net-snk/app/netapps/ping.c +++ b/clients/net-snk/app/netapps/ping.c @@ -163,6 +163,12 @@ ping(int argc, char *argv[]) } else { memcpy(&fn_ip.own_ip, &ping_args.client_ip.integer, 4); + if (ping_args.gateway_ip.integer) { + uint32_t netmask; + netmask = ping_args.gateway_ip.integer & ping_args.client_ip.integer; + set_ipv4_netmask(netmask & 0xFFFFFF00); + set_ipv4_router(ping_args.gateway_ip.integer); + } arp_failed = 1; printf(" Own IP address: "); }
ping was failing for machine across the subnet with statically assinged IP address. The parsed gateway address was ignored in the stack because the router variable was not set. Currently, ping does not take a netmask parameter, set netmask parameter with the common top order bits of client IP and gateway IP. Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> --- clients/net-snk/app/netapps/ping.c | 6 ++++++ 1 file changed, 6 insertions(+)