2
\$\begingroup\$

This is the updated version of the script first posted here.

Specifically, I first wanted to save the logs to a log file at "$HOME/.logs/ffupgrade.log", but I'm not sure whether that is good practice or whether I should log to syslog. When logging to a file, I would add another variable logfile at the beginning and adapt the functions like so (similarly for success and warning):

logfile="$HOME/.logs/ffupgrade.log"

error_exit() {
    msg="$1"
    echo -e "$(date) ${red}ERROR:${nocolor} ${msg}" 1>&2
    echo "$(date) ERROR: $msg" >> "$logfile"
    exit 1
}

I'm also wondering if it's better to only run the commands that need it with sudo or if I should rerun the whole script as sudo after prompting for the password like this:

if [[ "$EUID" != 0 ]]; then
    warning "Please run as root"
    sudo "$0" "$@"
fi

As always, any other comments are welcome as well.

#!/bin/bash

# firefox_upgrade - program to upgrade firefox quantum

red="\033[1;31m"
yellow="\033[1;33m"
green="\033[1;32m"
nocolor="\033[0m"

error_exit() {
    msg="$1"
    echo -e "$(date) ${red}ERROR:${nocolor} ${msg}" 1>&2
    logger "ERROR: $msg"
    exit 1
}

warning() {
    msg="$1"
    echo -e "$(date) ${yellow}WARNING:${nocolor} ${msg}"
    logger "WARNING: $msg"
}

success() {
    msg="$1"
    echo -e "$(date) ${green}SUCCESS:${nocolor} ${msg}"
    logger "SUCCESS: $msg"
}

ffbin="/opt/firefox"
fflink="/usr/bin/firefox-quantum"

ffpath=
fffile=

# check if path argument was provided
if [ $# -ne 1 ]; then
    error_exit "usage: $0 firefox_quantum_path"
fi

echo "Checking for root access.."
if [[ "$EUID" = 0 ]]; then
    success "already root"
else
    sudo -k # make sure to ask for password on next sudo
    if sudo true; then
        success "correct password"
    else
        error_exit "wrong password"
    fi
fi

# parsing path and filename
ffpath="$1"
fffile="${ffpath##*/}"

# check if input is a valid file
if [ ! -f "$ffpath" ]; then
    error_exit "Invalid file! Aborting."
fi

# check the firefox version
ffpref="${fffile%.*}"
ffversion="${ffpref//[^0-9]/}"
ffinstalled_version="$($fflink --version | sed 's/[^0-9]*//g')"

vind=0
if [[ "$ffversion" < "$ffinstalled_version" ]]; then
    warning "The version to be installed is lower than the installed version"
    vind=1
elif [[ "$ffversion" = "$ffinstalled_version" ]]; then
    warning "The version to be installed is the same as the installed version"
    vind=1
fi

if [[ "$vind" = 1 ]]; then
    echo "Do you want to continue anyway?"
    select yn in "Yes" "No"; do
        case $yn in
            Yes ) : ;;
            No ) exit;;
        esac
    done
fi

# removing previous install, if existent
if [ -e "$ffbin" ]; then
    sudo rm -rf "$ffbin"
    #mv "$ffbin" "$ffbin.old"
    success "removed previous install"
else
    warning "$ffbin doesn't exist."
fi

# removing previous symlink, if existent
if [ -L "$fflink" ]; then
    sudo rm -f "$fflink"
    success "removed previous symlink"
else
    warning "$fflink doesn't exist."
fi

# copying the tar to /opt
if ! sudo rsync -ah --progress "$ffpath" "/opt/$fffile"; then
    error_exit "Couldn't copy the tarball. Aborting."
fi
success "tarball copied"

# unpacking the tar if successfully changed directory
if ! sudo tar -jxvf "/opt/$fffile" -C /opt; then
    error_exit "Could not extract file! Aborting."
fi
success "tarball was extracted"

# if unpack was successful, set permissions, create symlink, and remove tar
sudo chmod 755 "$ffbin"
success "permissions set"
sudo ln -s "$ffbin/firefox" "$fflink"
success "symlink created"
sudo rm "/opt/$fffile"
success "tarball removed from /opt"
success "Firefox was upgraded"
```
\$\endgroup\$
  • 1
    \$\begingroup\$ Questions need to be self contained. Post all necessary context needed to review the code you have in your question please. \$\endgroup\$ – πάντα ῥεῖ Jul 29 at 18:31
  • 1
    \$\begingroup\$ What is missing from the question? \$\endgroup\$ – iuvbio Jul 29 at 19:36
  • 2
    \$\begingroup\$ I don't understand. Am I supposed to post the complete first version in this question? That would be quite a lot of code and not really add any value when you could just look at the previous question. Or are you referring to the logfile? If so, I will edit it to include a complete example of how that would look. \$\endgroup\$ – iuvbio Jul 29 at 19:40
  • 1
    \$\begingroup\$ Let's start with your title: "Debian Firefox Quantum update script v2" What does that mean?? \$\endgroup\$ – πάντα ῥεῖ Jul 29 at 19:43
  • 2
    \$\begingroup\$ Furthermore, I've asked you what exactly is missing, in your opinion, from the context of the question, as I'd be happy to provide additional detail. You have failed to provide an answer to that offering no explanation. I'd say that's pretty poor commentary. \$\endgroup\$ – iuvbio Jul 29 at 21:11
3
\$\begingroup\$

To sudo or not to sudo

I'm also wondering if it's better to only run the commands that need it with sudo or if I should rerun the whole script as sudo after prompting for the password like this:

if [[ "$EUID" != 0 ]]; then
    warning "Please run as root"
    sudo "$0" "$@"
fi

Can the script do its job without root permissions? This script cannot, because it wants to modify files that are normally only editable by root, such as /opt and /usr/bin. Therefore, the script requires root permissions. A good way to handle this is to check if the user has root permission, and exit if that's not the case, for example:

if [[ "$EUID" != 0 ]]; then
    warning "Please run as root"
    exit 1
fi

There's really no need to include the complexity of getting root permission in the script. With the above simple check, the user can either switch to the root user, or rerun the script with sudo. The rest of the script can assume the privilege (and responsibility) of root.

About logging

I first wanted to save the logs to a log file at "$HOME/.logs/ffupgrade.log", but I'm not sure whether that is good practice or whether I should log to syslog.

I don't really know. But I can reason about it.

Does this really belong to syslog? The script upgrades Firefox. Let's look at something similar, for example, what happens when you upgrade software using the standard package manager of a Linux system. I just upgraded something in a Debian system I have at hand, and it didn't log anything about it in syslog. It may be relevant whether the upgrade is happening on explicit user action or automatically. I would conclude that when a user upgrades a package manually, it's not noteworthy enough to add to syslog.

On the other hand, what happens if you keep logging to ~/.logs? It accumulates forever without cleanup. You might want to log somewhere else that is subject to log a rotation policy in your systems by default, or document how to set it up in the script. This could be a valid argument, except that I doubt your Firefox upgrade logs will ever accumulate to anything significant. So... I would not bother about rotation. If the script is only going to run as root, then I would log somewhere more visible than ~/.logs, let's say /var/log.

When logging to a file, I would add another variable logfile at the beginning and adapt the functions like so (similarly for success and warning)

I would add a logger function to encapsulate that logic, overriding the logger program, and then you won't need to touch the other functions.

Use local for local variables

It's good to use local for local variables in functions to avoid setting values in the global scope, which can be a source of nasty errors.

Double quote variables used on the command line

$fflink here should be enclosed in double-quotes:

ffinstalled_version="$($fflink --version | sed 's/[^0-9]*//g')"

Comparing versions

I have a doubt about comparing versions. The script removes all non-numeric characters, and then compares values lexicographically. Are you sure that will work reliably? It would have been good to include some example versions in comments, to help assure readers.

Also, to remove non-numeric characters, I would use tr -cd 0-9 instead of sed 's/[^0-9]*//g'.

Verify exit codes

The script does some things, and tells some things to the user that might not be true, for example here:

sudo chmod 755 "$ffbin"
success "permissions set"
sudo ln -s "$ffbin/firefox" "$fflink"
success "symlink created"
sudo rm "/opt/$fffile"
success "tarball removed from /opt"
success "Firefox was upgraded"

Even if those commands fail, the script will print success messages, and happily carry on.

Use better variable names

What is vind?

Use verbose mode more

I would add -v to all interesting commands that support it, for example rm, mv.

Replace 1>&2 with >&2

The default file descriptor redirected is 1, no need to spell it out explicitly.

Simple is better than complex

[ $# != 1 ] is probably more natural and therefore easier to understand than [ $# -ne 1 ].

\$\endgroup\$
  • \$\begingroup\$ Thanks. If you could add something concerning logger vs. logfile (I have added more detail to the question), I will accept this answer. As for the versions, you are right, there is the theoretical possibility that the version might contain letters as well, I will have to think of a better way to extract that. \$\endgroup\$ – iuvbio Jul 29 at 22:18
  • \$\begingroup\$ @iuvbio I added a section about it ;-) \$\endgroup\$ – janos Jul 30 at 5:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.