patternbashMinor
Bash script for setting up a LAN to WLAN router
Viewed 0 times
scriptlanroutersettingforbashwlan
Problem
I wrote a bash script for the Raspberry Pi 3 (Raspbian) which has the main task of setting up a LAN to WLAN router. In addition it makes some things nicer for the intended users who have Windows background only. It will be used on a Pi 3 with built-in WLAN and the official 7" display only.
Preconditions to run the script are (typically done on Windows, so my script can't help here):
The script shall
The order of those things is not really important, but it seems that the Ethernet configuration should only be done when no Internet access is needed any more, since the script might disable Internet access if the original connection was made over LAN.
Yeah, that's it. My tests seem ok, but since this is my first larger bash script, I'd like to get some feedback on how to improve things in the future.
```
#!/bin/sh
echo "Setting up this device as a LAN to WLAN gateway..."
[ -f /boot/splash.png ] && echo "Splash screen found" || { echo "Please copy splash.png"; exit 1; }
[ -f /boot/asplash ] && echo "Splash screen script found" || { echo "Please copy a file called asplash"; exit 1; }
ping 8.8.8.8 -c 1 -q -w 1 > /dev/null && echo "Internet connection detected" || { echo "Make sure you have a valid Internet connection." ; exit 1;
Preconditions to run the script are (typically done on Windows, so my script can't help here):
- download latest Raspbian
- flash to SD card
- copy 3 files to the /boot partition: the script, a file
asplashandsplash.png
- insert in Pi and run Raspbian
- configure an Internet connection
- run the script (
/boot/script.sh)
The script shall
- update the sources (
apt-get update)
- upgrade the installation (
apt-get upgrade)
- install required packages to make all of the following work
- set keyboard to German
- set timezone to Berlin
- forward IPv4 from LAN (eth0) to WLAN (wlan0)
- configure LAN (eth0) to 192.168.63.1 so it can act as a gateway on the 192.168.63.0/24 network
- replace the Raspberry boot output by a nice logo so that the user knows it'll be the right thing on startup
- set the Desktop background by the same nice logo
The order of those things is not really important, but it seems that the Ethernet configuration should only be done when no Internet access is needed any more, since the script might disable Internet access if the original connection was made over LAN.
Yeah, that's it. My tests seem ok, but since this is my first larger bash script, I'd like to get some feedback on how to improve things in the future.
```
#!/bin/sh
echo "Setting up this device as a LAN to WLAN gateway..."
[ -f /boot/splash.png ] && echo "Splash screen found" || { echo "Please copy splash.png"; exit 1; }
[ -f /boot/asplash ] && echo "Splash screen script found" || { echo "Please copy a file called asplash"; exit 1; }
ping 8.8.8.8 -c 1 -q -w 1 > /dev/null && echo "Internet connection detected" || { echo "Make sure you have a valid Internet connection." ; exit 1;
Solution
Creating files
For the record, the single biggest issue in this script is what @Sirac already mentioned: use here-documents instead of multiple
It's simpler, easier to read, and more efficient.
This script has no use as a normal user.
All the operations it provides only make sense with root access.
As such, instead of littering it with
it would be better to drop all the
and instead run this script itself with
Don't use
Using
For example here:
No need to
Avoid too long lines
On this line it's important that the script will exit if the condition fails, but I have to scroll to the right to see that:
It would be better to break the line using
But in this particular example, since you have multiple similar statements, I'd introduce a helper function:
When written this way, even though I don't see the end of the line without scrolling, I see "fatal", from which I can guess that there's not much interesting to see to scroll to the right, and I can simply read on to the next line.
Simplifying
Some of the
This is equivalent to:
You can avoid the duplication by using capture groups and a back reference:
Redundant commands
The first
The second one will set the executable flag for all, so you can simply delete the first line.
For the record, the single biggest issue in this script is what @Sirac already mentioned: use here-documents instead of multiple
echo statements to create a file.It's simpler, easier to read, and more efficient.
sudo everywhereThis script has no use as a normal user.
All the operations it provides only make sense with root access.
As such, instead of littering it with
sudo everywhere,it would be better to drop all the
sudo,and instead run this script itself with
sudo.Don't use
sudo willy-nillyUsing
sudo when it's not needed creates the bad habit of using it more than necessary, which defeats the purpose, and can lead to disaster.For example here:
sudo echo "XKBMODEL=\"pc105\"" | sudo tee /etc/default/keyboard > /dev/nullNo need to
echo text as root. Only the tee needs to be run as root.Avoid too long lines
On this line it's important that the script will exit if the condition fails, but I have to scroll to the right to see that:
[ -f /boot/splash.png ] && echo "Splash screen found" || { echo "Please copy splash.png"; exit 1; }It would be better to break the line using
\.But in this particular example, since you have multiple similar statements, I'd introduce a helper function:
fatal() {
echo "$*"
exit 1
}
[ -f /boot/splash.png ] && echo "Splash screen found" || fatal "Please copy splash.png"When written this way, even though I don't see the end of the line without scrolling, I see "fatal", from which I can guess that there's not much interesting to see to scroll to the right, and I can simply read on to the next line.
Simplifying
sedSome of the
sed commands can be written simpler:sudo sed -i -e '/net.ipv4.ip_forward/ s/.*/net.ipv4.ip_forward=1/' /etc/sysctl.confThis is equivalent to:
sudo sed -i -e 's/.*net.ipv4.ip_forward.*/net.ipv4.ip_forward=1/' /etc/sysctl.confYou can avoid the duplication by using capture groups and a back reference:
sudo sed -i -e 's/.*\(net.ipv4.ip_forward\).*/\1=1/' /etc/sysctl.confRedundant commands
The first
chmod here is unnecessary:sudo chmod +x /etc/network/if-pre-up.d/iptables
sudo chmod 755 /etc/network/if-pre-up.d/iptablesThe second one will set the executable flag for all, so you can simply delete the first line.
Code Snippets
sudo echo "XKBMODEL=\"pc105\"" | sudo tee /etc/default/keyboard > /dev/null[ -f /boot/splash.png ] && echo "Splash screen found" || { echo "Please copy splash.png"; exit 1; }fatal() {
echo "$*"
exit 1
}
[ -f /boot/splash.png ] && echo "Splash screen found" || fatal "Please copy splash.png"sudo sed -i -e '/net.ipv4.ip_forward/ s/.*/net.ipv4.ip_forward=1/' /etc/sysctl.confsudo sed -i -e 's/.*net.ipv4.ip_forward.*/net.ipv4.ip_forward=1/' /etc/sysctl.confContext
StackExchange Code Review Q#140056, answer score: 4
Revisions (0)
No revisions yet.