HiveBrain v1.2.0
Get Started
← Back to all entries
patternbashMinor

Bash script for setting up a LAN to WLAN router

Submitted by: @import:stackexchange-codereview··
0
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):

  • download latest Raspbian



  • flash to SD card



  • copy 3 files to the /boot partition: the script, a file asplash and splash.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 echo statements to create a file.
It's simpler, easier to read, and more efficient.

sudo everywhere

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 sudo everywhere,
it would be better to drop all the sudo,
and instead run this script itself with sudo.

Don't use sudo willy-nilly

Using 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/null


No 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 sed

Some of the sed commands can be written simpler:

sudo sed -i -e '/net.ipv4.ip_forward/ s/.*/net.ipv4.ip_forward=1/' /etc/sysctl.conf


This is equivalent to:

sudo sed -i -e 's/.*net.ipv4.ip_forward.*/net.ipv4.ip_forward=1/' /etc/sysctl.conf


You 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.conf


Redundant 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/iptables


The 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.conf
sudo sed -i -e 's/.*net.ipv4.ip_forward.*/net.ipv4.ip_forward=1/' /etc/sysctl.conf

Context

StackExchange Code Review Q#140056, answer score: 4

Revisions (0)

No revisions yet.