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

[Nikunj A Dadhania]

Alexey Kardashevskiy <aik at ozlabs.ru> writes:

> 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?

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