vnc-ssh-tunnel.sh: Script to connect to server over ssh tunnel#67
vnc-ssh-tunnel.sh: Script to connect to server over ssh tunnel#67ckorn wants to merge 1 commit intoany1:masterfrom
Conversation
any1
left a comment
There was a problem hiding this comment.
In the style of commit messages used for this project and many others, people would normally write something like "Add script for connecting over ssh tunnel".
It's also customary to wrap git messages to 72 characters.
scripts/vnc-ssh-tunnel.sh
Outdated
|
|
||
| show_help() | ||
| { | ||
| echo "usage: $(basename $BASH_SOURCE) [-p|--ssh-port port] host vnc_port" |
There was a problem hiding this comment.
Did you know about heredoc? Instead of repeating the echos, you can do this:
cat <<END_OF_TEXT
usage: do-a-thing [options] <whatever>
This command does a thing with some options and whatnot.
Options:
--stuff Do stuff
--help Get help!
END_OF_TEXT
There was a problem hiding this comment.
Thanks. Changed the code accordingly.
scripts/vnc-ssh-tunnel.sh
Outdated
| exit 1 | ||
| } | ||
|
|
||
| free_port() |
There was a problem hiding this comment.
Nit: better call this find_free_port
There was a problem hiding this comment.
Thanks. Changed the code accordingly.
scripts/vnc-ssh-tunnel.sh
Outdated
| local start=49152 | ||
| local range=5000 | ||
| while true; do | ||
| local port=$[$start + ($RANDOM % $range)] |
There was a problem hiding this comment.
Just c/p. Changed it to incrementaly.
scripts/vnc-ssh-tunnel.sh
Outdated
| local host="$1" | ||
| local port="$2" | ||
|
|
||
| if [ -z "$host" ]; then |
There was a problem hiding this comment.
Thanks. Changed the code accordingly.
scripts/vnc-ssh-tunnel.sh
Outdated
| fi | ||
| shift; shift | ||
|
|
||
| master_file="$(mktemp -d)/wlvncc" |
There was a problem hiding this comment.
These should be local.
The cleanup is missing for the temp directory.
There was a problem hiding this comment.
locals added.
The cleanup seems to be done automatically.
Also the trap code outputs "Control socket connect(/tmp/tmp.n89FXnzOHT/wlvncc): No such file or directory" so I added 2>/dev/null
There was a problem hiding this comment.
The cleanup seems to be done automatically
The ssh process might clean up the control socket, but I find it very unlikely that it would remove this directory for you. That would be very dangerous.
There was a problem hiding this comment.
Yeah, I can no longer reproduce this. Added rm -rf for the temp dir in the trap.
scripts/vnc-ssh-tunnel.sh
Outdated
| { | ||
| POSITIONAL_ARGS=() | ||
| local remote_port="22" | ||
| while [[ $# -gt 0 ]]; do |
There was a problem hiding this comment.
It's better to use getopt(1) or no option parsing at all. Home brew solutions almost always behave in surprising ways. E.g. here you would expect --ssh-port=1337 to behave the same way as --ssh-port 1337.
There was a problem hiding this comment.
Thanks. Used getopt here. Took the example code from: /usr/share/doc/util-linux/getopt-example.bash
|
Hopefully the script is fine now. My usual bash scripts are not longer than three lines but wanted to give it a try anyways ^^ Should I rebase my branch to delete the first commit? |
any1
left a comment
There was a problem hiding this comment.
Yes, fixup commits should always be squashed into whatever commit it is that they're fixing.
scripts/vnc-ssh-tunnel.sh
Outdated
| local start=49152 | ||
| local range=5000 | ||
| local i=0 | ||
| while true; do |
There was a problem hiding this comment.
A simpler way to iterate over a range would look like this:
let start=49152
let range=5000
for ((i = start; i < start + range; ++i)); do
scripts/vnc-ssh-tunnel.sh
Outdated
|
|
||
| main() | ||
| { | ||
| TEMP=$(getopt -o 'hp:' --long 'help,ssh-port:' -n "$(basename $BASH_SOURCE)" -- "$@") |
There was a problem hiding this comment.
Isn't option parsing overkill for this? Also, I believe that TEMP could really be a local.
Generally, I would just keep a shell script like this simple and just use environment variables for options. I.e. if you wanted a different port, you would have to call SSH_PORT=2200 vnc-ssh-tunnel.sh.
There was a problem hiding this comment.
That is what I wanted to ask anyway. Because with the current solution I can no longer forward options like -e raw to the actual process.
Will rewrite it with environment variables.
0623287 to
577543d
Compare
It is recommended the server only allowed connections from localhost. Therefore a ssh port forwarding connection to the local port on the server is required. This script opens a port forwarding connection to the server via ssh. wlvncc then connects to this local port. When the script terminates the ssh connection is closed. Closes: any1#32
|
Ok, finally I think to have fixed everything. |
It is recommended the server only allowed connections from localhost. Therefore a ssh port forwarding connection to the local port on the server is required. This script opens a port forwarding connection to the server via ssh. wlvncc then connects to this local port. When the script terminates the ssh connection is closed.
Thanks to @any1 for the initial script.
Closes: #32