Review compatibility, readability, good code standards, security.
This is for development server, where webmaster uses GUI fo IDE and fine way to create quickly apache2 virtualhost using GUI but fallback to text interface is present. Second review of script with applying previous review and adding more features.
I`ll post code parts fist, then whole script and git link to it.
If user runs script without terminal I need to notify him somehow about errors. xmessage provided with X but looks ugly, but looks compatible. May be suggested other way - portable and better looking. User will always get error output in terminal what's fine. In any case if using GUI user suggested to install zenity as a main gui tool of script - that will be message from xmessage.
notify_user () {
echo "1ドル" >&2
[ -t 0 ] || if type -p notify-send >/dev/null; then notify-send "1ドル"
else xmessage -buttons Ok:0 -nearmouse "1ドル" -timeout 10; fi
}
Is following exit 1 exits function or script? I guess only function. That's why empty input not break script, at least tested in giu. How to rewrite this function to handle empty input as error to break script?
get_virtual_host() {
if [ -t 0 ]; then
read -p "Create virtualhost (= Folder name,case sensitive)" -r host
else
host=$(zenity --forms --add-entry=Name --text='Create virtualhost (= Folder name,case sensitive)')
fi
case "$host" in
"") notify_user "Bad input: empty" ; exit 1 ;;
*"*"*) notify_user "Bad input: wildcard" ; exit 1 ;;
*[[:space:]]*) notify_user "Bad input: whitespace" ; exit 1 ;;
esac
echo "$host"
}
host=$(get_virtual_host)
What is better, hardcode some config string variables, allow input parameters[with leak verification], or make install script what hardcodes input parameters for server, also make .desktop file? I think hardcode is good, as one could hardcode and make permissions to edit and execute. Looks following:
webmaster="leonid" # user who access web files. group is www-data
maindomain=".localhost" # domain for creating subdomains, leading dot required
serveradmin="webmaster@localhost" # admin email
webroot="/home/${webmaster}/Web/" # root folder where subfolders for virtualhosts created
Full script: (git: https://github.com/LeonidMew/CreateVirtualHost)
#!/bin/bash
webmaster="leonid" # user who access web files. group is www-data
maindomain=".localhost" # domain for creating subdomains, leading dot required
serveradmin="webmaster@localhost" # admin email
webroot="/home/${webmaster}/Web/" # root folder where subfolders for virtualhosts created
a2ensite="050-" # short prefix for virtualhost config file
apachehost="/etc/apache2/sites-available/${a2ensite}" # prefix for virtualhost config file
tmphost=$(mktemp) # temp file for edit config
trap "rm $tmphost" EXIT # rm temp file on exit
notify_user () {
echo "1ドル" >&2
[ -t 0 ] || if type -p notify-send >/dev/null; then notify-send "1ドル"; else xmessage -buttons Ok:0 -nearmouse "1ドル" -timeout 10; fi
}
if [ -t 0 ];then
usegui=""
else
if ! type -p zenity >/dev/null;then
notify_user "Use terminal or install zenity for gui. '$ sudo apt install zenity'"
exit 1
else
usegui="yes"
fi
fi
# if [ "$(id -un)" == "root" ]
# then
# notify_user "You should not run this script as root but as user going to edit web files."
# exit 1
# fi
get_virtual_host() {
if [ -t 0 ]; then
read -p "Create virtualhost (= Folder name,case sensitive)" -r host
else
host=$(zenity --forms --add-entry=Name --text='Create virtualhost (= Folder name,case sensitive)')
fi
case "$host" in
"") notify_user "Bad input: empty" ; exit 1 ;;
*"*"*) notify_user "Bad input: wildcard" ; exit 1 ;;
*[[:space:]]*) notify_user "Bad input: whitespace" ; exit 1 ;;
esac
echo "$host"
}
host=$(get_virtual_host)
hostfile="${apachehost}${host}.conf" # apache virtualhost config file
dir="${webroot}${host}" # folder used as document root for virtualhost
# virtualhost template
cat >"$tmphost" <<EOF
<VirtualHost *:80>
ServerAdmin $serveradmin
DocumentRoot $dir
ServerName ${host}${maindomain}
ServerAlias ${host}${maindomain}
<Directory "$dir">
AllowOverride All
Require local
</Directory>
# Available loglevels: trace8, ..., trace1, debug, info, notice, warn,
# error, crit, alert, emerg.
LogLevel error
</VirtualHost>
# vim: syntax=apache ts=4 sw=4 sts=4 sr noet
EOF
if [ ! -z "$usegui" ];then
# edit virtualhost config
text=$(zenity --text-info --title="virtualhost config" --filename="$tmphost" --editable)
if [ -z "$text" ]
then
# cancel button pressed
exit 0
fi
echo "$text" > "$tmphost"
else
# edit virtualhost config
editor=${VISUAL:-$EDITOR}
if [ -z "$editor" ];then
if type -p nano >/dev/null;then editor="nano"; fi
fi
if [ -z "$editor" ];then
if type -p vim >/dev/null;then editor="vim"; fi
fi
if [ -z "$editor" ];then
echo "edit '$tmphost' to your liking, then hit Enter"
read -p "I'll wait ... "
else
"$editor" "$tmphost"
fi
fi
# probably want some validating here that the user has not broken the config
# apache will not reload config if incorrect
getsuperuser () {
if [ ! -z "$usegui" ];then
echo "pkexec"
else
echo "sudo"
fi
}
notify_user "execute root commands with $(getsuperuser) to create virtualhost"
$(getsuperuser) /bin/bash <<EOF
mkdir -p "$dir"
chown ${webmaster}:www-data "$dir"
chmod u=rwX,g=rX,o= "$dir"
cp "$tmphost" "$hostfile"
chown root:root "$hostfile"
chmod u=rw,g=r,o=r "$hostfile"
a2ensite "${a2ensite}${host}.conf"
systemctl reload apache2
EOF
notify_user "Virtualhost added. Apache2 reloaded."
1 Answer 1
Don't repeat yourself
Some snippets appear repeatedly in the code, for example:
if type -p some-command >/dev/null; then ...
Make it a function:
have_command() {
type -p "1ドル" >/dev/null
}
And then you can write more naturally:
if have_command notify-send; then
notify-send ...
fi
The same for checking if running in terminal:
in_terminal() {
[ -t 0 ]
}
Avoid excessively long lines
It can be annoying to have to scroll to the right to see what this line is about:
[ -t 0 ] || if type -p notify-send >/dev/null; then notify-send "1ドル"; else xmessage -buttons Ok:0 -nearmouse "1ドル" -timeout 10; fi
On top of that, I think an if statement is easiest to read in this form:
if ...; then
...
else
...
fi
Lastly, I think it's hard to read when an if statement is chained after a previous command with ||.
I would find this easier to read:
in_terminal && return
if have_command notify-send; then
notify-send "1ドル"
else
xmessage -buttons Ok:0 -nearmouse "1ドル" -timeout 10
fi
Use more functions
This piece of code tries to detect an editor to use, by checking multiple alternatives:
editor=${VISUAL:-$EDITOR} if [ -z "$editor" ];then if type -p nano >/dev/null;then editor="nano"; fi fi if [ -z "$editor" ];then if type -p vim >/dev/null;then editor="vim"; fi fi if [ -z "$editor" ];then echo "edit '$tmphost' to your liking, then hit Enter" read -p "I'll wait ... " else "$editor" "$tmphost" fi
What's not great about this is if the first check succeeds, the other checks still run.
I mean, if EDITOR is already defined, then if [ -z "$editor" ] will still be executed 3 times for nothing.
This is a good opportunity to use a function with early returns:
find_editor() {
local editor=${VISUAL:-$EDITOR}
if [ "$editor" ]; then
echo "$editor"
return
fi
for cmd in nano vim; then
if have_command "$cmd"; then
echo "$cmd"
return
fi
done
}
editor=$(find_editor)
if [ -z "$editor" ]; then
echo "edit '$tmphost' to your liking, then hit Enter"
read -p "I'll wait ... "
else
"$editor" "$tmphost"
fi
Simplify conditions
This condition is written more complicated than it needs to be:
if [ ! -z "$usegui" ];then
You can simplify to:
if [ "$usegui" ]; then
Missed double-quoting
You did a good job double-quoting variables used as command arguments. Here's one place you missed:
trap "rm $tmphost" EXIT
This would be better written as:
trap 'rm "$tmphost"' EXIT