[SLOF] [PATCH v3 2/3] ping: add netmask in the ping argument
Nikunj A Dadhania
nikunj at linux.vnet.ibm.com
Tue May 3 17:59:05 AEST 2016
Thomas Huth <thuth at redhat.com> writes:
> On 03.05.2016 06:01, Nikunj A Dadhania wrote:
>> The current ping command does not take netmask as argument, updated the
>> ping command to take "client-ip/nn" format ip address.
>>
>> Add routine to return netmask(class based), when not provided by user.
>>
>> Signed-off-by: Nikunj A Dadhania <nikunj at linux.vnet.ibm.com>
>> ---
>> clients/net-snk/app/netapps/args.c | 44 ++++++++++++++++++++++++++++++++++----
>> clients/net-snk/app/netapps/args.h | 1 +
>> clients/net-snk/app/netapps/ping.c | 21 ++++++++++++++++--
>> clients/net-snk/app/netlib/ipv4.c | 21 ++++++++++++++++++
>> clients/net-snk/app/netlib/ipv4.h | 1 +
>> slof/fs/loaders.fs | 4 ++--
>> 6 files changed, 84 insertions(+), 8 deletions(-)
>>
>> diff --git a/clients/net-snk/app/netapps/args.c b/clients/net-snk/app/netapps/args.c
>> index 52215e6..ed25c7c 100644
>> --- a/clients/net-snk/app/netapps/args.c
>> +++ b/clients/net-snk/app/netapps/args.c
>> @@ -98,20 +98,22 @@ argncpy(const char *arg_str, unsigned int index, char *buffer,
>> }
>>
>> /**
>> - * Converts "255.255.255.255" -> char[4] = { 0xff, 0xff, 0xff, 0xff }
>> + * Converts "255.255.255.255/nn" -> char[4] = { 0xff, 0xff, 0xff, 0xff }
>> + * *netmask = subnet_netmask(nn)
>> *
>> * @param str string to be converted
>> * @param ip in case of SUCCESS - 32-bit long IP
>> - in case of FAULT - zero
>> + * in case of FAULT - zero
>> + * @param netmask return netmask if there is a valid /nn encoding in IP
>> * @return TRUE - IP converted successfully;
>> * FALSE - error condition occurs (e.g. bad format)
>> */
>> int
>> -strtoip(const char *str, char ip[4])
>> +strtoip_netmask(const char *str, char ip[4], unsigned int *netmask)
>> {
>> char octet[10];
>> int res;
>> - unsigned int i = 0, len;
>> + unsigned int i = 0, len, has_nn = 0;
>>
>> while (*str != 0) {
>> if (i > 3 || !isdigit(*str))
>> @@ -123,6 +125,14 @@ strtoip(const char *str, char ip[4])
>> strncpy(octet, str, len);
>> octet[len] = 0;
>> str += len;
>> + } else if (strstr(str, "\\") != NULL) {
>> + len = (short) (strstr(str, "\\") - str);
>
> Shouldn't that be a forward slash instead? Or will this cause trouble
> with the slash handling from the Forth code? (in the latter case, you
> should at least adapt the 'Converts "255.255.255.255/nn"' comment in
> front of this function, I think)
Yes, forward slash has problem with parsing, even the file handling for
tftpboot uses backward slash. I will update the comment accordingly.
>
> Anyway, you could also use strchr() instead of strstr() as a small
> optimization, I think.
>
>> + if (len >= 10)
>> + return 0;
>> + strncpy(octet, str, len);
>> + octet[len] = 0;
>> + str += len;
>> + has_nn = 1;
>> } else {
>> strncpy(octet, str, 9);
>> octet[9] = 0;
>> @@ -135,9 +145,35 @@ strtoip(const char *str, char ip[4])
>> i++;
>> if (*str == '.')
>> str++;
>> + if(has_nn) {
>> + str++;
>> + strncpy(octet, str, 9);
>> + octet[9] = 0;
>> + res = strtol(octet, NULL, 10);
>> + str += strlen(octet);
>> + if (res > 31 || res < 1)
>> + return 0;
>> + if (netmask)
>> + *netmask = (0xFFFFFFFFUL << (32 - res)) & 0xFFFFFFFFUL;
>
> I think you could simply do without the final "& 0xFFFFFFFFUL" here.
Being more careful :-)
> And for the first 0xFFFFFFFF, you also don't need the UL suffix, do
> you?
Should be fine.
>
>> + }
>> }
>>
>> if (i != 4)
>> return 0;
>> return -1;
>> }
>> +
>> +/**
>> + * Converts "255.255.255.255" -> char[4] = { 0xff, 0xff, 0xff, 0xff }
>> + *
>> + * @param str string to be converted
>> + * @param ip in case of SUCCESS - 32-bit long IP
>> + in case of FAULT - zero
>> + * @return TRUE - IP converted successfully;
>> + * FALSE - error condition occurs (e.g. bad format)
>> + */
>> +int
>> +strtoip(const char *str, char ip[4])
>> +{
>> + return strtoip_netmask(str, ip, NULL);
>> +}
>> diff --git a/clients/net-snk/app/netapps/args.h b/clients/net-snk/app/netapps/args.h
>> index b80982a..1ede9a8 100644
>> --- a/clients/net-snk/app/netapps/args.h
>> +++ b/clients/net-snk/app/netapps/args.h
>> @@ -18,5 +18,6 @@ unsigned int get_args_count(const char *);
>> unsigned int get_arg_length(const char *);
>> char *argncpy(const char *, unsigned int, char *, unsigned int);
>> int strtoip(const char *, char[4]);
>> +int strtoip_netmask(const char *, char[4], unsigned int *netmask);
>>
>> #endif /* _ARGS_H */
>> diff --git a/clients/net-snk/app/netapps/ping.c b/clients/net-snk/app/netapps/ping.c
>> index 4facf06..bd8f2d3 100644
>> --- a/clients/net-snk/app/netapps/ping.c
>> +++ b/clients/net-snk/app/netapps/ping.c
>> @@ -35,13 +35,15 @@ struct ping_args {
>> unsigned int integer;
>> } gateway_ip;
>> unsigned int timeout;
>> + /* Netmask 192.168.1.10/24 format */
>
> What do you mean with "192.168.1.10/24 format" here? You store here the
> real netmask, not the prefix length, right?
You are right, my earlier code was storing just th /nn part, which i had
later changed to netmask, will update the comment.
>
>> + unsigned int netmask;
>> };
>>
>> static void
>> usage(void)
>> {
>> printf
>> - ("\nping device-path:[device-args,]server-ip,[client-ip],[gateway-ip][,timeout]\n");
>> + ("\nping device-path:[device-args,]server-ip,[client-ip[\\nn]],[gateway-ip][,timeout]\n");
>> }
>>
>> @@ -82,7 +84,7 @@ parse_args(const char *args, struct ping_args *ping_args)
>> }
>>
>> argncpy(args, 0, buf, 64);
>> - if (!strtoip(buf, ping_args->client_ip.string)) {
>> + if (!strtoip_netmask(buf, ping_args->client_ip.string, &ping_args->netmask)) {
>> /* this should have been the client (our) IP address */
>> return -1;
>> } else {
>> @@ -112,6 +114,7 @@ ping(int argc, char *argv[])
>> int fd_device;
>> struct ping_args ping_args;
>> uint8_t own_mac[6];
>> + uint32_t netmask;
>>
>> memset(&ping_args, 0, sizeof(struct ping_args));
>>
>> @@ -163,6 +166,14 @@ ping(int argc, char *argv[])
>>
>> } else {
>> memcpy(&fn_ip.own_ip, &ping_args.client_ip.integer, 4);
>> + if (!ping_args.netmask) {
>> + /* Netmask is not provided, assume default according to
>> + * the network class
>> + */
>> + ping_args.netmask = get_default_ipv4_netmask(ping_args.client_ip.string);
>> + }
>> + set_ipv4_netmask(ping_args.netmask);
>> +
>> arp_failed = 1;
>> printf(" Own IP address: ");
>> }
>> @@ -174,6 +185,12 @@ ping(int argc, char *argv[])
>> ((fn_ip.own_ip >> 24) & 0xFF), ((fn_ip.own_ip >> 16) & 0xFF),
>> ((fn_ip.own_ip >> 8) & 0xFF), (fn_ip.own_ip & 0xFF));
>>
>> + if ((netmask = get_ipv4_netmask())) {
>
> Just a matter of taste - but could you maybe write that in two lines:
>
> netmask = get_ipv4_netmask();
> if (netmask) ...
>
>> + printf(" Netmask : ");
>> + printf("%d.%d.%d.%d\n", ((netmask >> 24) & 0xFF), ((netmask >> 16) & 0xFF),
>> + ((netmask >> 8) & 0xFF), (netmask & 0xFF));
>> + }
>> +
>> memcpy(&fn_ip.server_ip, &ping_args.server_ip.integer, 4);
>> printf(" Ping to %d.%d.%d.%d ", ((fn_ip.server_ip >> 24) & 0xFF),
>> ((fn_ip.server_ip >> 16) & 0xFF),
>> diff --git a/clients/net-snk/app/netlib/ipv4.c b/clients/net-snk/app/netlib/ipv4.c
>> index 2b92c77..3a1a789 100644
>> --- a/clients/net-snk/app/netlib/ipv4.c
>> +++ b/clients/net-snk/app/netlib/ipv4.c
>> @@ -250,6 +250,27 @@ uint32_t get_ipv4_netmask(void)
>> }
>>
>> /**
>> + * IPv4: Get the default subnet mask according to the IP class
>> + *
>> + * @param ip_addr IPv4 address
>> + * @return default netmask according to the IP class
>> + */
>> +uint32_t get_default_ipv4_netmask(char *ip_addr)
>> +{
>> + unsigned char top;
>> +
>> + top = ip_addr[0];
>> + if (top > 0 && top < 128)
>> + return 0xFF000000; /* Class A: 255.0.0.0 */
>> + else if (top >= 128 && top < 192)
>> + return 0xFFFF0000; /* Class B: 255.255.0.0 */
>> + else if (top >= 192 && top < 224)
>> + return 0xFFFFFF00; /* Class C: 255.255.255.0 */
>> + else
>> + return 0;
>> +}
>> +
>> +/**
>> * IPv4: Creates IP-packet. Places IP-header in a packet and fills it
>> * with corresponding information.
>> * <p>
>> diff --git a/clients/net-snk/app/netlib/ipv4.h b/clients/net-snk/app/netlib/ipv4.h
>> index 18821ea..5717c9a 100644
>> --- a/clients/net-snk/app/netlib/ipv4.h
>> +++ b/clients/net-snk/app/netlib/ipv4.h
>> @@ -69,6 +69,7 @@ extern void set_ipv4_router(uint32_t router_ip);
>> extern uint32_t get_ipv4_router(void);
>> extern void set_ipv4_netmask(uint32_t subnet_mask);
>> extern uint32_t get_ipv4_netmask(void);
>> +extern uint32_t get_default_ipv4_netmask(char *ip_addr);
>>
>> extern int (*send_ip) (int fd, void *, int);
>>
>> diff --git a/slof/fs/loaders.fs b/slof/fs/loaders.fs
>> index 1478c70..8ae502d 100644
>> --- a/slof/fs/loaders.fs
>> +++ b/slof/fs/loaders.fs
>> @@ -70,7 +70,7 @@ CREATE load-list 2 cells allot load-list 2 cells erase
>> THEN
>> ;
>>
>> -: ping ( "{device-path:[device-args,]server-ip,[client-ip],[gateway-ip][,timeout]}" -- )
>> +: ping ( "{device-path:[device-args,]server-ip,[client-ip[\nn]],[gateway-ip][,timeout]}" -- )
>> my-self >r current-node @ >r \ Save my-self
>> (parse-line) open-dev dup IF
>> dup to my-self dup ihandle>phandle set-node
>> @@ -83,7 +83,7 @@ CREATE load-list 2 cells allot load-list 2 cells erase
>> swap close-dev
>> ELSE
>> cr
>> - ." Usage: ping device-path:[device-args,]server-ip,[client-ip],[gateway-ip][,timeout]"
>> + ." Usage: ping device-path:[device-args,]server-ip,[client-ip[\nn]],[gateway-ip][,timeout]"
>> cr drop
>> THEN
>> r> set-node r> to my-self \ Restore my-self
>>
>
> Apart from the nits, the patch looks fine to me.
Will send a patch with updates
>
> Thomas
Regards
Nikunj
More information about the SLOF
mailing list