Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dist/tools/ethos: add setup_network.sh script #11816

Merged
merged 1 commit into from
Jul 15, 2019

Conversation

kaspar030
Copy link
Contributor

Contribution description

This PR provides an alternative to start_network.sh.

start_network.sh is supposed to be use as TERMPROG and needs to be started as root.
It sets up a tap device, starts uhcpd in background, then launches ethos.

This PR's setup_network.sh drops starting ethos and starts uhcpd in foreground.

The use-case is:

  • run sudo setup_network.sh <params> in one terminal, keep running
  • run make term/test in another terminal, re-using the existing network

Testing procedure

This is used in the soon-to-be-openened SUIT PR. I suggest using that PR as testbed.

Issues/PRs references

@kaspar030 kaspar030 added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: tools Area: Supplementary tools labels Jul 8, 2019
@kaspar030 kaspar030 requested a review from miri64 July 8, 2019 11:38
@fjmolinas
Copy link
Contributor

fjmolinas commented Jul 8, 2019

FYI in #11808 I also makes use of this script.

kaspar030 added a commit to kaspar030/RIOT that referenced this pull request Jul 8, 2019
kaspar030 added a commit to kaspar030/RIOT that referenced this pull request Jul 10, 2019
kaspar030 added a commit to kaspar030/RIOT that referenced this pull request Jul 10, 2019
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on #11808 and #11818, I was able to run make test using ethos without using sudo. There is quite some code duplication between setup_network.sh and start_network.sh. I feel they can probably factorized, something like this:

diff --git a/dist/tools/ethos/setup_network.sh b/dist/tools/ethos/setup_network.sh
index baee92b2d..7fab50a99 100755
--- a/dist/tools/ethos/setup_network.sh
+++ b/dist/tools/ethos/setup_network.sh
@@ -14,8 +14,8 @@ remove_tap() {
     ip tuntap del ${TAP} mode tap
 }
 
-cleanup() {
-    echo "Cleaning up..."
+cleanup_setup() {
+    echo "Cleaning up setup  ..."
     remove_tap
     ip a d fd00:dead:beef::1/128 dev lo
     trap "" INT QUIT TERM EXIT
@@ -43,7 +43,6 @@ if [ -z "$_USER" ]; then
     fi
 fi
 
-trap "cleanup" INT QUIT TERM EXIT
-
+trap "cleanup_setup" INT QUIT TERM EXIT
 
 create_tap && start_uhcpd
diff --git a/dist/tools/ethos/start_network.sh b/dist/tools/ethos/start_network.sh
index 928f05453..fc90e6b2e 100755
--- a/dist/tools/ethos/start_network.sh
+++ b/dist/tools/ethos/start_network.sh
@@ -2,31 +2,14 @@
 
 ETHOS_DIR="$(dirname $(readlink -f $0))"
 
-create_tap() {
-    ip tuntap add ${TAP} mode tap user ${USER}
-    sysctl -w net.ipv6.conf.${TAP}.forwarding=1
-    sysctl -w net.ipv6.conf.${TAP}.accept_ra=0
-    ip link set ${TAP} up
-    ip a a fe80::1/64 dev ${TAP}
-    ip a a fd00:dead:beef::1/128 dev lo
-    ip route add ${PREFIX} via fe80::2 dev ${TAP}
-}
-
-remove_tap() {
-    ip tuntap del ${TAP} mode tap
-}
-
 cleanup() {
     echo "Cleaning up..."
-    remove_tap
-    ip a d fd00:dead:beef::1/128 dev lo
-    kill ${UHCPD_PID}
+    kill -TERM $(pgrep -f "uhcpd" | xargs)
     trap "" INT QUIT TERM EXIT
 }
 
-start_uhcpd() {
-    ${UHCPD} ${TAP} ${PREFIX} > /dev/null &
-    UHCPD_PID=$!
+setup_network() {
+    ${ETHOS_DIR}/setup_network.sh $1 $2 &
 }
 
 PORT=$1
@@ -46,5 +29,4 @@ UHCPD="$(readlink -f "${ETHOS_DIR}/../uhcpd/bin")/uhcpd"
 
 trap "cleanup" INT QUIT TERM EXIT
 
-
-create_tap && start_uhcpd && "${ETHOS_DIR}/ethos" ${TAP} ${PORT} ${BAUDRATE}
+setup_network ${TAP} ${PREFIX} && "${ETHOS_DIR}/ethos" ${TAP} ${PORT} ${BAUDRATE}


Travis is complaining about a trailing white space in the README. Can you add a some doc in the README specifying the difference between setup_network.sh and start_network.sh (I know it is not really related but its weird having only one of them documented).

kaspar030 added a commit to kaspar030/RIOT that referenced this pull request Jul 12, 2019
kaspar030 added a commit to kaspar030/RIOT that referenced this pull request Jul 12, 2019
@fjmolinas
Copy link
Contributor

@kaspar030 ping

@kaspar030 kaspar030 force-pushed the add_setup_network.sh branch from dff6886 to 6f1147a Compare July 15, 2019 09:08
@kaspar030
Copy link
Contributor Author

Travis is complaining about a trailing white space in the README.

fixed

@kaspar030
Copy link
Contributor Author

I feel they can probably factorized, something like this:

I've integrated your proposal, with minor changes. (e.g., start_network.sh is now remembering the PID of the called setup_network, instead of blindly killing all uhcpds on the system).

But this needs now to be tested with existing users of start_network.sh...

@kaspar030
Copy link
Contributor Author

But this needs now to be tested with existing users of start_network.sh...

How about we split this deduplication into a seperate PR, so we can focus on #11818 first?

@fjmolinas
Copy link
Contributor

But this needs now to be tested with existing users of start_network.sh...

What kind of testing? Does this need to be considered as an API change? The functionality is the same and the way the script is called is the same, this could only cause problems for user that are sourcing the script and using the defined functions right?

How about we split this deduplication into a seperate PR, so we can focus on #11818 first?

I would be Ok with this.

@kaspar030
Copy link
Contributor Author

What kind of testing? Does this need to be considered as an API change? The functionality is the same and the way the script is called is the same

yeah, but the implementation is slightly different. So if start_network.sh works as before needs to be tested, e.g., with the border router example.

I would be Ok with this.

done, #11840

@fjmolinas
Copy link
Contributor

yeah, but the implementation is slightly different. So if start_network.sh works as before needs to be tested, e.g., with the border router example.

OK!

Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested using #11808 and #11818, even though this change add some code duplication they will be addressed in #11840. ACK!

@kaspar030 kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 15, 2019
@fjmolinas
Copy link
Contributor

@kaspar030 this one still needs squashing!

@kaspar030 kaspar030 force-pushed the add_setup_network.sh branch from a3ec33d to f74e5e7 Compare July 15, 2019 10:33
@kaspar030
Copy link
Contributor Author

@kaspar030 this one still needs squashing!

thx, done!

@fjmolinas
Copy link
Contributor

GO!

@fjmolinas fjmolinas merged commit 8121daa into RIOT-OS:master Jul 15, 2019
@kaspar030 kaspar030 deleted the add_setup_network.sh branch July 15, 2019 11:39
@kaspar030
Copy link
Contributor Author

Thanks!

@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants