patternbashMinor
Yum Notification Script
Viewed 0 times
scriptyumnotification
Problem
This is a Bash script I wrote to email me when updates are available for my server. I'm new to bash programming, so I don't know if I've done this well.
```
#!/bin/bash
# Copyright 2016 FlyingPiMonster
#
# THIS SOFTWARE IS RELEASED "AS IS", WITH NO WARRANTY OR GUARANTEE OF FITNESS FOR A PARTICULAR PURPOSE. THE DEVELOPER IS NOT RESPONSIBLE FOR ANY DAMAGE
# CAUSED BY THE USE OF THIS SOFTWARE.
#
# All rights reserved.
#
# Set up argument variables
reload_cron=false
print_help=false
quiet_mode=false
print_config=false
# Read arguments using getopts
while getopts ":rhqd" opts; do
case "$opts" in
r)
reload_cron=true ;;
\?|h)
print_help=true ;;
q)
quiet_mode=true ;;
d)
print_config=true ;;
esac
done
# Set up default configuration options
typeset -A config
config=(
[EmailFrom]="YumNotifier"
[EmailRecipient]="root@localhost"
[EmailSubject]="Updates available for your computer"
[EmailTitle]="Updates Available"
[EmailGreeting]="YumNotifier has detected updates for some software on your computer."
[EmailSignature]="Thank you for using YumNotifier!"
[HideUpdateDetails]="No"
[EmailHiddenUpdateText]="Please login with SSH and use the yum command to install these updates."
[RunFrequency]="DAILY"
[UserId]="root"
[ErrorBadConfigurationOption]='The following configuration option is not set correctly:'
[ErrorWritingCrontab]="Could not write to crontab file"
[ErrorSendingMail]="An error occurred while sending mail"
[MessageNoUpdatesAvailable]="No updates available"
)
# Read configuration file
while read -r line; do
if echo "$line" | grep -F = | grep "^[^#]" | &>/dev/null; then
varname=$(echo "$line" | cut -d '=' -f 1)
config["$varname"]=$(echo "$line" | cut -d '=' -f 2)
fi
done /etc/cron.d/yumnotifier.cron) &>/dev/null
code="$?"
if [[ "$code" != 0 ]]; then
if [[ "$quiet_mode" == "false" ]]; then
echo "${con
```
#!/bin/bash
# Copyright 2016 FlyingPiMonster
#
# THIS SOFTWARE IS RELEASED "AS IS", WITH NO WARRANTY OR GUARANTEE OF FITNESS FOR A PARTICULAR PURPOSE. THE DEVELOPER IS NOT RESPONSIBLE FOR ANY DAMAGE
# CAUSED BY THE USE OF THIS SOFTWARE.
#
# All rights reserved.
#
# Set up argument variables
reload_cron=false
print_help=false
quiet_mode=false
print_config=false
# Read arguments using getopts
while getopts ":rhqd" opts; do
case "$opts" in
r)
reload_cron=true ;;
\?|h)
print_help=true ;;
q)
quiet_mode=true ;;
d)
print_config=true ;;
esac
done
# Set up default configuration options
typeset -A config
config=(
[EmailFrom]="YumNotifier"
[EmailRecipient]="root@localhost"
[EmailSubject]="Updates available for your computer"
[EmailTitle]="Updates Available"
[EmailGreeting]="YumNotifier has detected updates for some software on your computer."
[EmailSignature]="Thank you for using YumNotifier!"
[HideUpdateDetails]="No"
[EmailHiddenUpdateText]="Please login with SSH and use the yum command to install these updates."
[RunFrequency]="DAILY"
[UserId]="root"
[ErrorBadConfigurationOption]='The following configuration option is not set correctly:'
[ErrorWritingCrontab]="Could not write to crontab file"
[ErrorSendingMail]="An error occurred while sending mail"
[MessageNoUpdatesAvailable]="No updates available"
)
# Read configuration file
while read -r line; do
if echo "$line" | grep -F = | grep "^[^#]" | &>/dev/null; then
varname=$(echo "$line" | cut -d '=' -f 1)
config["$varname"]=$(echo "$line" | cut -d '=' -f 2)
fi
done /etc/cron.d/yumnotifier.cron) &>/dev/null
code="$?"
if [[ "$code" != 0 ]]; then
if [[ "$quiet_mode" == "false" ]]; then
echo "${con
Solution
No violations on shellcheck.net, nicely done!
I do have a couple of recommendations.
Splitting a line in two
This is an inefficient way to split a line into two parts:
Two subshells,
It would be better to use pattern substitution instead,
staying within the same process:
But there is a caveat. If there are multiple
this will not take the 2nd column like
However, if that's a serious concern, it's possible to work around that with a few more intermediary substitutions.
The point is to execute as few programs as possible in the process.
Shortening the pipeline
For the same reason as in the previous point,
it's good to keep pipelines short.
For example here, the two
Like this:
One less process to execute.
The
What is that last
It seems unnecessary.
Redirecting
But does
I've never seen it.
So I think the only practical concern is
There's a flag
so the line can be simplified to:
The
The
When comparing to a literal string,
it's slightly clearer to use
So instead of this:
I recommend this:
Magic numbers and strings
so that if you ever need to change it, you can do it in one place.
It could be a good idea to put those in variables too.
Inconsistent indentation
The indentation is inconsistent at places.
It would be better to make it consistent.
I do have a couple of recommendations.
Splitting a line in two
This is an inefficient way to split a line into two parts:
varname=$(echo "$line" | cut -d '=' -f 1)
config["$varname"]=$(echo "$line" | cut -d '=' -f 2)Two subshells,
echo and cut processes in each.It would be better to use pattern substitution instead,
staying within the same process:
varname=${line%%=*}
config["$varname"]=${line##*=}But there is a caveat. If there are multiple
= signs on the line,this will not take the 2nd column like
cut does.However, if that's a serious concern, it's possible to work around that with a few more intermediary substitutions.
The point is to execute as few programs as possible in the process.
Shortening the pipeline
For the same reason as in the previous point,
it's good to keep pipelines short.
For example here, the two
grep commands could be combined:if echo "$line" | grep -F = | grep "^[^#]" | &>/dev/null; thenLike this:
if echo "$line" | grep "^[^#].*=" | &>/dev/null; thenOne less process to execute.
The
echo can also be removed by using a here-string instead:if grep "^[^#].*=" /dev/null; thenWhat is that last
| doing there before the redirection?It seems unnecessary.
Redirecting
stdout and stderr of grep&>/dev/null after the grep redirects both stdout and stderr to /dev/null.But does
grep ever produce something on stderr?I've never seen it.
So I think the only practical concern is
stdout.There's a flag
-q to make that easy,so the line can be simplified to:
if grep -q "^[^#].*=" <<< "$line"; thenThe
== operator of [[ ... ]]The
== operator is useful for pattern matching.When comparing to a literal string,
it's slightly clearer to use
= instead.So instead of this:
if [[ "$print_help" == "true" ]]; thenI recommend this:
if [[ "$print_help" = "true" ]]; thenMagic numbers and strings
exit 78 appears twice. I suggest putting 78 in a variable,so that if you ever need to change it, you can do it in one place.
true and false are magic strings.It could be a good idea to put those in variables too.
Inconsistent indentation
The indentation is inconsistent at places.
It would be better to make it consistent.
Code Snippets
varname=$(echo "$line" | cut -d '=' -f 1)
config["$varname"]=$(echo "$line" | cut -d '=' -f 2)varname=${line%%=*}
config["$varname"]=${line##*=}if echo "$line" | grep -F = | grep "^[^#]" | &>/dev/null; thenif echo "$line" | grep "^[^#].*=" | &>/dev/null; thenif grep "^[^#].*=" <<< "$line" | &>/dev/null; thenContext
StackExchange Code Review Q#136545, answer score: 2
Revisions (0)
No revisions yet.