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

Yum Notification Script

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

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:

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; then


Like this:

if echo "$line" | grep "^[^#].*=" | &>/dev/null; then


One less process to execute.

The echo can also be removed by using a here-string instead:

if grep "^[^#].*=" /dev/null; then


What 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"; then


The == 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" ]]; then


I recommend this:

if [[ "$print_help" = "true" ]]; then


Magic 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; then
if echo "$line" | grep "^[^#].*=" | &>/dev/null; then
if grep "^[^#].*=" <<< "$line" | &>/dev/null; then

Context

StackExchange Code Review Q#136545, answer score: 2

Revisions (0)

No revisions yet.