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: deduplicate setup_network.sh, start_network.sh #11840

Closed
wants to merge 5 commits into from

Conversation

kaspar030
Copy link
Contributor

Contribution description

#11816 introduced setup_network.sh, introducing some duplication with start_network.sh.
This PR aims to remove the duplications.

Testing procedure

Issues/PRs references

@kaspar030 kaspar030 added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: tools Area: Supplementary tools labels Jul 15, 2019
@kaspar030 kaspar030 requested a review from fjmolinas July 15, 2019 09:24
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 this still works on #11818, tests is passing as before.

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 start_network.sh by running the test in #11818 using wireless nodes. The BR was an iotlab-m3.

I also tested gnrc_networking on a samr21-xpro with the same BR and I was able to ping the samr21-xpro from my local host.

If you agree with my changes then squash directly.

remove_tap
ip a d fd00:dead:beef::1/128 dev lo
kill ${UHCPD_PID}
[ -n "${SETUP_NETWORK_PID}" ] && kill -TERM ${SETUP_NETWORK_PID}
Copy link
Contributor

Choose a reason for hiding this comment

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

This fails to kill all the decedent processes, I would suggest this change:

diff --git a/dist/tools/ethos/start_network.sh b/dist/tools/ethos/start_network.sh
index 4f832a258..a457735e0 100755
--- a/dist/tools/ethos/start_network.sh
+++ b/dist/tools/ethos/start_network.sh
@@ -3,13 +3,13 @@
 ETHOS_DIR="$(dirname $(readlink -f $0))"
 
 cleanup() {
-    [ -n "${SETUP_NETWORK_PID}" ] && kill -TERM ${SETUP_NETWORK_PID}
     trap "" INT QUIT TERM EXIT
+    [ -n "${SETUP_NETWORK_GPID}" ] && kill -TERM -$SETUP_NETWORK_GPID
 }
 
 setup_network() {
     ${ETHOS_DIR}/setup_network.sh $1 $2 &
-    SETUP_NETWORK_PID=$!
+    SETUP_NETWORK_GPID=$(ps -o pgid= $! | grep -o '[0-9]*')
 }
 
 PORT=$1

@fjmolinas
Copy link
Contributor

ping @kaspar030

1 similar comment
@fjmolinas
Copy link
Contributor

ping @kaspar030

@fjmolinas
Copy link
Contributor

ping @kaspar030!

@fjmolinas
Copy link
Contributor

this should be a low hanging fruit..

@stale
Copy link

stale bot commented Jun 7, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Jun 7, 2020
@miri64
Copy link
Member

miri64 commented Jun 8, 2020

Ping @kaspar030

@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Jun 8, 2020
@kaspar030 kaspar030 requested a review from jia200x as a code owner June 8, 2020 12:33
@kaspar030
Copy link
Contributor Author

this should be a low hanging fruit..

honestly I'm lost on this one, rebasing is messy due to the DHCPv6 additions. Does one of you want to take over (or re-do it)?

@miri64
Copy link
Member

miri64 commented Jun 9, 2020

this should be a low hanging fruit..

honestly I'm lost on this one, rebasing is messy due to the DHCPv6 additions. Does one of you want to take over (or re-do it)?

Was planning on that anyway (also to integrate with tapsetup) some time. Let's close this as memo archived for now then?

@kaspar030 kaspar030 closed this Jun 9, 2020
@miri64 miri64 added the State: archived State: The PR has been archived for possible future re-adaptation label Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: tools Area: Supplementary tools State: archived State: The PR has been archived for possible future re-adaptation Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants