[SLOF] [PATCH 1/2] ping: use gateway address for routing

[Alexey Kardashevskiy]

On 04/22/2016 05:39 PM, Nikunj A Dadhania wrote:
> Thomas Huth <thuth at redhat.com> writes:
>
>> On 22.04.2016 07:23, Nikunj A Dadhania wrote:
>>> Thomas Huth <thuth at 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 at 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