shell_commands: adapt ping6 to original-ping-like implementation #9523

miri64 · 2018-07-09T15:10:50Z

Contribution description

Loosely based on the original ping and netutil's ping

New features (compared to old RIOT version):

Non-positional parameters
Configurable hop-limit
Better duplicate detection (addresses shell ping6 doesn't display duplicate packages #9387)
Better asynchronous behavior
Potential for future move to sock_ip
(Optional) DNS-support
Multithreading-safe (in case shell-command handler gets called
from multiple threads)

Issues/PRs references

Resolves #9387.

miri64 · 2018-07-09T15:15:06Z

(I originally looked at the busybox-implementation [that's where the original title of this PR came from, because the commit had still the name ^^"], but since that is licensed under 4-clause BSD and thus incompatible to (L)GPL, I needed to dig deeper and was able to reconstruct my own version from the original ping's source [public domain; can be found on wayback machine] and inetutil's [licensed under GPLv3] and thus my version is now rather inspired by those works than the busybox one)

miri64 · 2018-07-09T18:47:47Z

For the record: this implementation is also more efficient than the old one. On a Cortex-M4 platform it uses ~200 byte less ROM and 56 byte less RAM than the previous implementation.

miri64 · 2018-07-09T18:49:54Z

(picked nucleo-f446ze to check that)

jcarrano · 2018-07-10T08:33:46Z

I'm running it without arguments and it's spitting a stream of random bytes to the terminal.

miri64 · 2018-07-10T08:37:52Z

I'm running it without arguments and it's spitting a stream of random bytes to the terminal.

On which application and which board?

miri64 · 2018-07-10T08:38:34Z

~~With gnrc_networking on native I don't have that:~~ sorry wrong branch....

> ping6
ping6
ping6 [<count>] <ipv6 addr>[%<interface>] [<payload_len>] [<delay in ms>] [<stats interval>]
defaults:
    count = 3
    interface = first interface if only one present, only needed for link-local addresses
    payload_len = 4
    delay = 1000
    stats interval = count

miri64 · 2018-07-10T08:39:36Z

mh curious, yesterday it was working...

miri64 · 2018-07-10T08:46:14Z

@jcarrano can you try again please?

jcarrano · 2018-07-10T09:17:03Z

@miri64 Yes, it's fixed. I could run it. I can't test hoplimit because my test setup is quite simple. I'll start looking at the code.

miri64 · 2018-07-10T09:46:08Z

@miri64 Yes, it's fixed. I could run it. I can't test hoplimit because my test setup is quite simple. I'll start looking at the code.

https://www.iot-lab.info/ ;-)

jcarrano

I'm submitting a partial review of only the parsing code. The network part will take me more time as I'm not familiar with the apis.

sys/shell/commands/sc_gnrc_icmpv6_echo.c

jcarrano · 2018-07-10T09:25:21Z

sys/shell/commands/sc_gnrc_icmpv6_echo.c

+    }
+    ++argv;
+    /* parse command line arguments */
+    for (; argc > 0; argc--, argv++) {


I find this simultaneous decrementing and incrementing kind of fragile. Why not use an explicit index?

For argc/argv I find this usually ok, but let me try with explicit index.

Ok, going for explicit index.

jcarrano · 2018-07-10T09:31:23Z

sys/shell/commands/sc_gnrc_icmpv6_echo.c

+              "of any responses, otherwise wait for two RTTs");
+}
+
+static int _configure(int argc, char **argv, _ping_data_t *data)


ping6 -c 3 without hostname fails. I have some quite lightweight code for parsing command line arguments that I wrote several years ago. If there's interest I can bring it back to life. I think there's no point in everyone hacking together their own parser.

I have some quite lightweight code for parsing command line arguments that I wrote several years ago. If there's interest I can bring it back to life. I think there's no point in everyone hacking together their own parser.

While I agree, but that opens a similar can of worms as the POSIX networking part, as some libc-variants already provide getopt and some don't. Since the shell is purely a debugging feature (and shell commands should also have as less dependencies as possible), I'd rather prefer every shell command to handle parsing for itself.

getopt posix-only, and does not support long options (getopt_long is a GNU extension).

I'd rather prefer every shell command to handle parsing for itself.

I would rephrase that as "I'd prefer every shell command to have it's own parsing bugs".

getopt posix-only, and does not support long options (getopt_long is a GNU extension).

At least my version of newlib supports it:

$ grep -R getopt /usr/arm-none-eabi/include/ /usr/arm-none-eabi/include/getopt.h:getopt.h - Read command line options /usr/arm-none-eabi/include/getopt.h:The getopt() function parses the command line arguments. Its arguments argc /usr/arm-none-eabi/include/getopt.h:The getopt_long() function works like getopt() except that it also accepts /usr/arm-none-eabi/include/getopt.h:The getopt() function returns the option character if the option was found /usr/arm-none-eabi/include/getopt.h:The getopt_long() function's return value is described below. /usr/arm-none-eabi/include/getopt.h:The function getopt_long_only() is identical to getopt_long(), except that a /usr/arm-none-eabi/include/getopt.h:getopt() and friends to return EOF with optind != argc. /usr/arm-none-eabi/include/getopt.h:This file and the accompanying getopt.c implementation file are hereby /usr/arm-none-eabi/include/getopt.h: int *flag; /* determines if getopt_long() returns a /usr/arm-none-eabi/include/getopt.h:/* While getopt.h is a glibc extension, the following are newlib extensions. /usr/arm-none-eabi/include/getopt.h: * They are optionally included via the __need_getopt_newlib flag. */ /usr/arm-none-eabi/include/getopt.h:#ifdef __need_getopt_newlib /usr/arm-none-eabi/include/getopt.h: allocated variable of type struct getopt_data. */ /usr/arm-none-eabi/include/getopt.h: #define getopt_r __getopt_r /usr/arm-none-eabi/include/getopt.h: #define getopt_long_r __getopt_long_r /usr/arm-none-eabi/include/getopt.h: #define getopt_long_only_r __getopt_long_only_r /usr/arm-none-eabi/include/getopt.h: /* The getopt_data structure is for reentrancy. Its members are similar to /usr/arm-none-eabi/include/getopt.h: typedef struct getopt_data /usr/arm-none-eabi/include/getopt.h: } getopt_data; /usr/arm-none-eabi/include/getopt.h:#endif /* __need_getopt_newlib */ /usr/arm-none-eabi/include/getopt.h: int _EXFUN (getopt, /usr/arm-none-eabi/include/getopt.h: int _EXFUN (getopt_long, /usr/arm-none-eabi/include/getopt.h: int _EXFUN (getopt_long_only, /usr/arm-none-eabi/include/getopt.h:#ifdef __need_getopt_newlib /usr/arm-none-eabi/include/getopt.h: int _EXFUN (__getopt_r, /usr/arm-none-eabi/include/getopt.h: struct getopt_data * __data)); /usr/arm-none-eabi/include/getopt.h: int _EXFUN (__getopt_long_r, /usr/arm-none-eabi/include/getopt.h: struct getopt_data * __data)); /usr/arm-none-eabi/include/getopt.h: int _EXFUN (__getopt_long_only_r, /usr/arm-none-eabi/include/getopt.h: struct getopt_data * __data)); /usr/arm-none-eabi/include/getopt.h:#endif /* __need_getopt_newlib */ /usr/arm-none-eabi/include/getopt.h:/* END OF FILE getopt.h */ /usr/arm-none-eabi/include/sys/unistd.h:# include <getopt.h> /usr/arm-none-eabi/include/sys/unistd.h:extern char *optarg; /* getopt(3) external variables */ /usr/arm-none-eabi/include/sys/unistd.h:int getopt(int, char * const [], const char *); /usr/arm-none-eabi/include/sys/unistd.h:extern int optreset; /* getopt(3) external variable */

Compare e.g. AVRlibc:

$ grep -R getopt /usr/lib/avr/include/

sys/shell/commands/sc_gnrc_icmpv6_echo.c

jcarrano · 2018-07-10T09:56:06Z

@miri64

Your registration has been saved and submitted to validation by an administrator.

miri64 · 2018-07-10T10:25:27Z

@miri64

Your registration has been saved and submitted to validation by an administrator.

Maybe @aabadie can speed up that process?

tcschmidt · 2018-07-11T18:34:52Z

@cgundogan maybe you can test this on IoTLab?

cgundogan · 2018-07-24T11:23:39Z

@miri64

Your registration has been saved and submitted to validation by an administrator.

@jcarrano were you able to perform the tests in a multihop setup? If not, I'd help you out here.

miri64 · 2018-08-16T14:15:31Z

ping @jcarrano

jcarrano · 2018-08-16T16:56:30Z

@miri64 I was not able to perform the test. I would have to learn how to use IOTLab and I'm lacking time ATM.

miri64 · 2018-08-16T18:49:15Z

@jcarrano you can also use two native nodes or two of the many IEEE 802.15.4 capable nodes on your desk ;-)

jcarrano · 2018-11-30T10:43:26Z

@miri64 Sorry for leaving this PR hanging. I have since given up reviewing network-related stuff which, not being in my main area of expertise, takes me too much time while I have little to offer in terms of a valuable review.

My own reviews in this PR were mainly related to command line parsing, and they have been addressed, so I will dismiss myself if you don't mind.

Comments were addressed.

miri64 · 2018-11-30T10:46:08Z

[…] so I will dismiss myself if you don't mind.

Sure. I understand.

miri64 · 2018-12-29T17:26:21Z

Rebased to current master. Also allowed for delay of 0 since I'm not sure why I disallowed that ;-).

gschorcht · 2019-01-05T17:35:39Z

@miri64 I have tested the ping6 command. It is working as expected. What about option -I to define the interface as implemented on UNIX machines? It is doing the same as as the %- notation.

miri64 · 2019-01-05T17:42:00Z

@miri64 I have tested the ping6 command. It is working as expected. What about option -I to define the interface as implemented on UNIX machines? It is doing the same as as the %- notation.

The man page to iputils ping says:

-I interface address
    Set source address to specified interface address. Argument may be numeric IP
    address or name of device. When pinging IPv6 link-local address this option is
    required.

So I would say it's redundant in a pure IPv6 scenario.

gschorcht · 2019-01-05T18:20:57Z

-I interface address
    Set source address to specified interface address. Argument may be numeric IP
    address or name of device. When pinging IPv6 link-local address this option is
    required.

So I would say it's redundant in a pure IPv6 scenario.

Yes, was just a question whether we should have it in addition to the %-notation of interface id.

miri64 · 2019-01-07T11:35:58Z

Yes, was just a question whether we should have it in addition to the %-notation of interface id.

I'd say no. One way to define the interface is enough IMHO (contrary to iputil's ping you can also use the % with a global address)

cgundogan · 2019-02-01T06:00:17Z

sys/shell/commands/sc_gnrc_icmpv6_echo.c

+                goto finish;
+            default:
+                /* requeue wrong packets */
+                msg_send(&msg, sched_active_pid);


What other messages can appear here? Shouldn't they be dropped instead of requeued? I can't see another msg_recv anywhere here that handles other types of messages, so the queue would get full with garbage ..

I can research that, but on first glance I would say: remember that this is the main thread. Other application specific messages might land in the queue.

(I did it because the current version in master does it as well:

RIOT/sys/shell/commands/sc_icmpv6_echo.c

Lines 302 to 305 in 1d69340

default:

/* requeue wrong packets */

msg_send(&msg, thread_getpid());

break;

)

remember that this is the main thread. Other application specific messages might land in the queue.

yes, sounds sensible. Please research that anyway (:

Ok, I had in memory, that this line existed because someone asked for it, but apparently I introduced it myself in #2555. Anyway, given that this is main and that we didn't face any problems with this until now, I think the line is here correctly.

cgundogan

Tested on native and iotlab-m3 with unicast and multicast addresses. The output looks sensible. ACK.
We should also scan the code base and tutorials/wiki to adjust for the new syntax. e.g. here.

cgundogan · 2019-02-04T11:16:32Z

squashing needed

miri64 · 2019-02-04T11:39:57Z

We should also scan the code base and tutorials/wiki to adjust for the new syntax. e.g. here.

We should this however do in a follow-up IMHO

miri64 · 2019-02-04T11:40:07Z

Squashed

Loosely based on [the original ping] and [netutil]'s ping New features (compared to old RIOT version): - non-positional parameters - Better duplicate detection (addresses RIOT-OS#9387) - Better asynchronous behavior - Potential for future move to `sock_ip` - (Optional) DNS-support - Multithreading-safe (in case shell-command handler gets called from multiple threads) [the original ping]: http://ftp.arl.army.mil/~mike/ping.html [netutil]: https://www.gnu.org/software/inetutils/

miri64 · 2019-02-04T11:43:16Z

And rebased.

cgundogan · 2019-02-05T15:42:36Z

Alright. Murdock also agrees!

miri64 · 2019-02-05T16:00:41Z

Tested on native and iotlab-m3 with unicast and multicast addresses. The output looks sensible. ACK.
We should also scan the code base and tutorials/wiki to adjust for the new syntax. e.g. here.

@cgundogan the wiki page you referenced is already marked as outdated. Should we really fix it there? (all other occurrences I found, I fixed).

miri64 · 2019-02-05T16:04:12Z

(it even mentions fibroute)

miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking GNRC labels Jul 9, 2018

miri64 changed the title ~~shell_commands: adapt ping6 to busybox-like implementation~~ shell_commands: adapt ping6 to original-ping-like implementation Jul 9, 2018

miri64 force-pushed the gnrc_icmpv6_echo/enh/new-shell-cmd branch from b887d79 to e36c0cd Compare July 9, 2018 15:11

miri64 force-pushed the gnrc_icmpv6_echo/enh/new-shell-cmd branch from e36c0cd to 2d73d12 Compare July 9, 2018 15:17

miri64 requested review from jcarrano and bergzand July 9, 2018 18:25

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 9, 2018

miri64 force-pushed the gnrc_icmpv6_echo/enh/new-shell-cmd branch from a0218ec to 5dac3d3 Compare July 9, 2018 18:56

jcarrano previously requested changes Jul 10, 2018

View reviewed changes

jcarrano mentioned this pull request Jul 10, 2018

sys/optparse: Add command line parser. #9538

Closed

miri64 mentioned this pull request Sep 12, 2018

shell ping6 doesn't display duplicate packages #9387

Closed

miri64 force-pushed the gnrc_icmpv6_echo/enh/new-shell-cmd branch from 5bbaa96 to 90a621b Compare December 29, 2018 17:25

miri64 mentioned this pull request Jan 4, 2019

gnrc: packet space in gnrc packet buffer is not released on very heavy load #10701

Closed

miri64 mentioned this pull request Jan 13, 2019

iotlab-m3: poor ping6 performance and a possibly broken 6lo fragmentation? #10761

Closed

cgundogan reviewed Feb 1, 2019

View reviewed changes

cgundogan approved these changes Feb 4, 2019

View reviewed changes

miri64 force-pushed the gnrc_icmpv6_echo/enh/new-shell-cmd branch from 90a621b to 1ec9591 Compare February 4, 2019 11:40

miri64 force-pushed the gnrc_icmpv6_echo/enh/new-shell-cmd branch from 1ec9591 to 45ec766 Compare February 4, 2019 11:43

cgundogan merged commit e8f89b9 into RIOT-OS:master Feb 5, 2019

miri64 deleted the gnrc_icmpv6_echo/enh/new-shell-cmd branch February 5, 2019 15:43

miri64 added this to the Release 2019.04 milestone Feb 5, 2019

miri64 mentioned this pull request Feb 5, 2019

Adapt for new ping6 command RIOT-OS/Release-Specs#102

Merged

danpetry mentioned this pull request Apr 9, 2019

Handle new IPv6 ping format; handle echoing tapbr0 interface jia200x/Release-Specs#12

Open

miri64 mentioned this pull request Apr 10, 2019

Release 2019.04 - RC1 RIOT-OS/Release-Specs#113

Closed

miri64 mentioned this pull request Oct 24, 2019

Release 2019.10 - RC2 RIOT-OS/Release-Specs#142

Closed

80 tasks

	default:
	/* requeue wrong packets */
	msg_send(&msg, thread_getpid());
	break;

shell_commands: adapt ping6 to original-ping-like implementation #9523

shell_commands: adapt ping6 to original-ping-like implementation #9523

Conversation

miri64 commented Jul 9, 2018 • edited Loading

Contribution description

Issues/PRs references

miri64 commented Jul 9, 2018

miri64 commented Jul 9, 2018

miri64 commented Jul 9, 2018

jcarrano commented Jul 10, 2018

miri64 commented Jul 10, 2018

miri64 commented Jul 10, 2018 • edited Loading

miri64 commented Jul 10, 2018

miri64 commented Jul 10, 2018

jcarrano commented Jul 10, 2018

miri64 commented Jul 10, 2018

jcarrano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcarrano commented Jul 10, 2018

miri64 commented Jul 10, 2018

tcschmidt commented Jul 11, 2018

cgundogan commented Jul 24, 2018

miri64 commented Aug 16, 2018

jcarrano commented Aug 16, 2018

miri64 commented Aug 16, 2018

jcarrano commented Nov 30, 2018

miri64 commented Nov 30, 2018

miri64 commented Dec 29, 2018

gschorcht commented Jan 5, 2019

miri64 commented Jan 5, 2019

gschorcht commented Jan 5, 2019

miri64 commented Jan 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgundogan left a comment

Choose a reason for hiding this comment

cgundogan commented Feb 4, 2019

miri64 commented Feb 4, 2019

miri64 commented Feb 4, 2019

miri64 commented Feb 4, 2019

cgundogan commented Feb 5, 2019

miri64 commented Feb 5, 2019

miri64 commented Feb 5, 2019

miri64 commented Jul 9, 2018 •

edited

Loading

miri64 commented Jul 10, 2018 •

edited

Loading

miri64 Jul 10, 2018 •

edited

Loading