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

native: unify and simplify usage of tapsetup script #3182

Merged
merged 2 commits into from
Aug 14, 2015

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Jun 9, 2015

Started with the intention to make the deactivation of IPv6 optional, ended up overhauling the whole script(s).

From the self-docs:

usage: tapsetup [OPTIONS]

  If no option is given, -c is assumed

  OPTIONS:
  -c [<num>], --create [<num>]: Create <num> tap interfaces (default: 2)
                  -d, --delete: Delete all interface
    -b <name>, --bridge <name>: Give name for the bridge (default: tapbr)
       -t <name>, --tap <name>: name base for the tap interfaces; the
                                generated names will be <name>x
                                (default: tap; ignored on OSX and FreeBSD)
         -6, --deactivate-ipv6: deactivate IPv6 for the interfaces and bridge
                                (ignored on OSX and FreeBSD)
                    -h, --help: prints this text

@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Platform: native Platform: This PR/issue effects the native platform labels Jun 9, 2015
@miri64 miri64 added this to the Release NEXT MAJOR milestone Jun 9, 2015
@miri64
Copy link
Member Author

miri64 commented Jun 9, 2015

Should work for Linux, Mac OS X, and FreeBSD. Please test.

DEACTIVATE_IPV6=""

usage() {
echo "usage: $PROGRAM [OPTIONS]" >&2
Copy link
Member

Choose a reason for hiding this comment

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

Please surround all variable names with curly braces, i.e. $PROGRAM -> ${PROGRAM}.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do. Any particular reason why?

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed

@miri64
Copy link
Member Author

miri64 commented Jun 12, 2015

(this removes the script's dependency on brutils BTW, since it uses iproute2 on Linux for bridge configuration instead of it).

echo " (default: tap; ignored on OSX and FreeBSD)" >&2
echo " -6, --deactivate-ipv6: deactivate IPv6 for the interfaces and bridge" >&2
echo " (ignored on OSX and FreeBSD)" >&2
echo " -h, --help: prints this text" >&2
Copy link
Member

Choose a reason for hiding this comment

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

please make the help options left-aligned

@LudwigKnuepfer
Copy link
Member

Something fails on FreeBSD:

~/RIOT/examples/default/bin/native/default.elf: open(/dev/tap0): Operation not permitted

@LudwigKnuepfer
Copy link
Member

set -x on FreeBSD output:

this PR:

+ basename ./cpu/native/tapsetup
+ PROGRAM=tapsetup
+ COUNT=2
+ COMMAND=''
+ BRNAME=tapbr0
+ TAPNAME=tap
+ DEACTIVATE_IPV6=''
+ true
+ BRNAME=bridge0
+ shift 2
+ true
+ break
+ [ -z lo ]
+ [ -z '' ]
+ COMMAND=create
+ uname -s
+ PLATFORM=FreeBSD
+ [ create = create ]
+ create_bridge
+ echo 'creating bridge0'
creating bridge0
+ sudo kldload if_bridge
+ sudo ifconfig bridge0 create
+ begin_tap
+ sudo kldload if_tap
+ seq 0 1
+ create_tap
+ echo 'creating tap0'
creating tap0
+ sudo ifconfig tap0 create
+ sudo chown lo /dev/tap0
+ sudo ifconfig tap0 up
+ sudo ifconfig bridge0 addm tap0
+ create_tap
+ echo 'creating tap1'
creating tap1
+ sudo ifconfig tap1 create
+ sudo chown lo /dev/tap1
+ sudo ifconfig tap1 up
+ sudo ifconfig bridge0 addm tap1
+ up_bridge
+ sudo ifconfig bridge0 up
+ basename ./cpu/native/tapsetup
+ PROGRAM=tapsetup
+ COUNT=2
+ COMMAND=''
+ BRNAME=tapbr0
+ TAPNAME=tap
+ DEACTIVATE_IPV6=''
+ true
+ BRNAME=bridge0
+ shift 2
+ true
+ [ COMMAND = create ]
+ COMMAND=delete
+ shift
+ true
+ break
+ [ -z lo ]
+ [ -z delete ]
+ uname -s
+ PLATFORM=FreeBSD
+ [ delete = create ]
+ [ delete = delete ]
+ delete_bridge
+ echo 'deleting bridge0'
deleting bridge0
+ sudo kldunload if_tap
+ sudo kldunload if_bridge
+ exit 0

master:

+ COMMAND=create
+ COUNT=''
+ DEFCOUNT=2
+ [ -z lo ]
+ [ -z create ]
+ [ create = create ]
+ [ -z '' ]
+ COUNT=2
+ sudo kldload if_tap
+ sudo kldload if_bridge
+ sudo sysctl net.link.tap.user_open=1
net.link.tap.user_open: 0 -> 1
+ echo 'creating  ...'
creating  ...
+ sudo ifconfig bridge0 create
+ sudo ifconfig bridge0 up
+ seq 0 1
+ sudo ifconfig tap0 create
+ sudo chown lo /dev/tap0
+ sudo ifconfig tap0 up
+ sudo ifconfig bridge0 addm tap0
+ sudo ifconfig tap1 create
+ sudo chown lo /dev/tap1
+ sudo ifconfig tap1 up
+ sudo ifconfig bridge0 addm tap1
+ exit 0
+ COMMAND=delete
+ COUNT=''
+ DEFCOUNT=2
+ [ -z lo ]
+ [ -z delete ]
+ [ delete = create ]
+ [ delete = delete ]
+ sudo sysctl net.link.tap.user_open=0
net.link.tap.user_open: 1 -> 0
+ sudo kldunload if_tap
+ sudo kldunload if_bridge
+ exit 0

@miri64
Copy link
Member Author

miri64 commented Jun 13, 2015

Can you explain to me, what this output means?

@miri64
Copy link
Member Author

miri64 commented Jun 13, 2015

adapted doc, addressed comments and (hopefully) fixed for OSX and FreeBSD.

@miri64 miri64 modified the milestones: POSIX compliance, Release NEXT MAJOR, Release 2015.06 Jun 15, 2015
@miri64
Copy link
Member Author

miri64 commented Jun 23, 2015

@LudwigOrtmann ping?

@OlegHahm
Copy link
Member

@LudwigOrtmann won't be avaliable until July 4.

@haukepetersen
Copy link
Contributor

When @authmillenon and me just talked about this PR, it came to my mind if it makes sense to move this script out of the cpu/native location to somewhere more 'toolish', say dist/tools/tapsetup/ or similar. What do you guys think about this?

@OlegHahm
Copy link
Member

👍

@miri64
Copy link
Member Author

miri64 commented Jun 23, 2015

@LudwigOrtmann won't be avaliable until July 4.

Can someone else test this script then on FreeBSD and OSX?

case "${PLATFORM}" in
FreeBSD)
sudo kldload if_tap || exit 1;;
sudo sysctl net.link.tap.user_open=1
Copy link
Member

Choose a reason for hiding this comment

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

my zsh is complaining here. Please move the ;; to the end of this case

Copy link
Member

Choose a reason for hiding this comment

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

/bin/sh is a symlink to zsh on your system?

Copy link
Member

Choose a reason for hiding this comment

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

no it's not (: But still, this script refuses to work without these fixes

@miri64
Copy link
Member Author

miri64 commented Jun 23, 2015

Addressed comments and fixed doc accordingly.

@miri64 miri64 force-pushed the native/enh/tapsetup branch from 99ad86a to f05efed Compare June 23, 2015 16:39
@miri64
Copy link
Member Author

miri64 commented Jul 1, 2015

@thomaseichinger and @emmanuelsearch any luck testing this on OSX? @phiros any luck testing this on FreeBSD?

@emmanuelsearch
Copy link
Member

I tested this on OS X, and it works as well as with #3290 (i.e. it sets up something, but ping6 timeouts 100% of the time in both directions). But I guess this PR was not focused on this issue. Basically: I think I can't test this more right now?

@miri64
Copy link
Member Author

miri64 commented Jul 9, 2015

Since I don't have access to a OSX machine a more technical test would be far more helpful. This means: use tests/dev_eth with txtsnd instead of ping and a clarification were the packet exactly gets lost. (Can you sniff the packets using wireshark or similar software? )

@miri64
Copy link
Member Author

miri64 commented Jul 9, 2015

Or do you observe the same behavior with the old version of the script too. In that case, you are right: this is not the scope of this PR.

@LudwigKnuepfer
Copy link
Member

Works on FreeBSD 10

@miri64
Copy link
Member Author

miri64 commented Jul 16, 2015

Can somebody ACK then?

@LudwigKnuepfer
Copy link
Member

AcK

@OlegHahm OlegHahm added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 20, 2015
@miri64 miri64 added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Jul 27, 2015
@miri64 miri64 force-pushed the native/enh/tapsetup branch from f05efed to 664f51e Compare August 14, 2015 10:05
@miri64
Copy link
Member Author

miri64 commented Aug 14, 2015

Rebased and squashed

miri64 added a commit that referenced this pull request Aug 14, 2015
native: unify and simplify usage of tapsetup script
@miri64 miri64 merged commit 99adcbe into RIOT-OS:master Aug 14, 2015
@miri64
Copy link
Member Author

miri64 commented Aug 14, 2015

And finally merged.

@miri64 miri64 deleted the native/enh/tapsetup branch August 14, 2015 10:55
@miri64 miri64 mentioned this pull request Aug 21, 2015
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties Platform: native Platform: This PR/issue effects the native platform Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants