6
\$\begingroup\$

I started self-learning Bash scripting yesterday, and I finished my first successful "advance" script.

Goal

To create an interactive step-by-step script to that allowed customizing various parts of the process of creating permanent swapfiles. I based my idea off of Ask Ubuntu Answer's #1 and #2.

Background

While creating it, I extended ideas such as allowing exiting from the script at any time and formatting the output.

You will see that I use different styles throughout the code (i.e. case vs if-elif-else, etc...). That deals with me learning as I coded, and having issues trying to get the syntax right while learning. I want to consolidate my different styles - probably into cases.

Additional Concerns I have

I know that all of you will suggest any issues that you see, and I will be happy about that; however, I am curious about a few things.

  1. Is this code portable? [Edit] Mostly anyone with a Linux Distro should be able to use this script.
  2. I put a lot of effort into formatting the output and making it look easy to read. I tried to separate the output of my code vs the output of other commands (i.e. adding >> and>>>>). Is it good enough? Would you suggest anything else? etc?

Possible Extensions

The code works as intended; however, I do plan on extending it in the future. These could affect possible responses - some response might make these harder or easier to implement, or they might have no effect at all. Anyways, if I extend the code later on to accommodate more use-cases, these are what I believe I could do in the future:

  1. Allow a non-interactive script mode that uses arguments for customizations
  2. Allow single/batch removal of swapfiles.
  3. Allow easier testing of each pathway of the interactive script
  4. Allow color and bold support.

Code

GitHub Repo at ciscorucinski/Swapfile-Utils

For unmodified code, check out the Protected Branch at 1_200606_StackExchange-CodeReview

Note: Only the variable declaration of help_text on the GitHub link is different due to a mistake on my side. No escaped tabs and newlines in GitHub

For updated code, check out the Development Branch at develop

#!/bin/bash

help_text="/n>> Documentation:\n\n\t

Usage: $(basename "$0") [-h]\n\t 
Creates a new permanent swapfile\n\n\t
where:\n\t\t
    -h  displays help text"


ask_for_size_allocation="
>>>> Amount to allocate for new swapfile? (default: 4G) : "

ask_for_swapfile_name="
>>>> Filename for new swapfile? (default: '/swapfile') : "

line="# # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # #"

shopt -s extglob

case $1 in
    ?(-)@(-h|-help) )
        echo -e help_text
        ;;
esac

retry=true
while $retry; do
    echo -e -n ">>>> Are you sure you want to create a new swapfile? (Y / N):"
    read yes_no
    case $yes_no in
        [Yy]|[Yy][Ee][Ss] )

            echo -e "\n$line"
            echo -e "Current Swapfiles:\n"
            sudo swapon -s 
            echo -e "$line"
            retry=false
            ;;
        [Nn]|[Nn][Oo]|[Qq][Uu][Ii][Tt] )
            echo -e ">> Exiting..."
            exit 0
            ;;
        * )
            echo -e ">> Error: invalid response\n"
            retry=true
    esac
done

echo -e ""
echo -e ">> Step 1: Size Allocation"
echo -e -n $ask_for_size_allocation
read swap_size

if [ -z "${swap_size}" ]; then
    swap_size="4G"
elif [[ $swap_size =~ [1-9][0-9]*[mMgG] ]]; then
    :
elif [[ $swap_size =~ [Qq][Uu][Ii][Tt] ]]; then
    echo -e ">> Exiting..."
    exit 0
else
    echo -e ">> Invalid Size: ${swap_size^^}. Exiting..."
    exit 1
fi

echo -e ""
echo -e ">> Step 2: File Name"
echo -e -n $ask_for_swapfile_name
read swap_name

if [ -z "${swap_name}" ]; then
    swap_name="/swapfile"
elif [[ $swap_size =~ [Qq][Uu][Ii][Tt] ]]; then
    echo -e ">> Exiting..."
    exit 0
elif [[ $swap_name =~ [/]+([0-9a-zA-Z]|[_-]) ]]; then
    :
elif [[ $swap_name =~ [+([0-9a-zA-Z]|[_-])] ]]; then 
    swap_name="/$swap_name" 
else
    echo -e ">> Invalid Pattern: $swap_name. Exiting..."
    exit 1
fi

echo -e ""
echo -e -n ">>>> Continue? '$swap_name' (${swap_size^^}) will be created. (Y / N):"
read yes_no
case $yes_no in
    [Yy]|[Yy][Ee][Ss] )
        echo -e""
        echo -e ">> 1. Creating swapfile..."

        echo -e ""
        echo -e "$line"

        sudo fallocate -l $swap_size $swap_name
        sudo chmod 600 $swap_name
        sudo mkswap $swap_name

        echo -e "$line"
        echo -e ""

        echo -e ">> 2. Enabling swapfile..."
        sudo swapon $swap_name

        echo -e ">> 3. Swapfile added."
        echo -e ""
        echo -e "$line"
        echo -e "Current Swapfiles:"
        echo -e ""
        sudo swapon -s 
        echo -e "$line"
        echo -e ""
        ;;
    [Qq][Uu][Ii][Tt]|[Nn]|[Nn][Oo]|[*])
        echo -e ">> Exiting..."
        exit 0
        ;;
esac

echo -e -n ">>>> Make swapfile permanent? (Y / N):"
read yes_no

case $yes_no in
    [Yy]|[Yy][Ee][Ss] )

        echo -e "$swap_name none swap sw 0 0" | sudo tee -a /etc/fstab > /dev/null

        echo -e ""
        echo -e ">> 4. Created permanent swapfile. Modified '/etc/fstab'"

        echo -e -n ">>>> Do you want to view '/etc/fstab?' (Y / N):"
            read yes_no

            case $yes_no in
                [Yy]|[Yy][Ee][Ss] )

                    lenght=${#swap_name}
                    echo -e ""
                    echo -e "$line"
                    cat /etc/fstab 
                    echo -e "$line"
                    ;;
                *)
                    echo -e ">> Exiting..."
                    exit 0
                    ;;
            esac
        ;;
    [Qq][Uu][Ii][Tt] )
        echo -e ">> Exiting..."
        exit 0  
        ;;
    *)
        echo -e ">> 4. Created temp swapfile."
        echo -e ">> Exiting..."
        exit 0
        ;;
esac

shopt -u extglob

Output

$ bash new_swapfile.sh
>>>> Are you sure you want to create a new swapfile? (Y / N):y

# # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # #
Current Swapfiles:

[sudo] password for ciscorucinski: 
Filename                Type        Size    Used    Priority
/swapfile_ext3                          file        262140  238384  -5
/swapfile_ext7                          file        131068  106376  -3
/swapfile                               file        8388604 476148  -6
/swapfile_ext6                          file        131068  68840   -4
/swapfile_ext5                          file        131068  115304  -2
/dev/dm-1                               partition   1003516 0   -7
/swapfile_ext8                          file        131068  0   -8
/swapfile_ext9                          file        131068  0   -9
/swapfile_ext10                         file        131068  0   -10
/swapfile_ext11                         file        131068  0   -11
/swapfile_ext12                         file        131068  0   -12
/swapfile_ext13                         file        131068  0   -13
/swapfile_ext                           file        524284  0   -14
/swapfile_ext14                         file        131068  0   -15
/swapfile_ext15                         file        131068  0   -16
# # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # #

>> Step 1: Size Allocation
>>>> Amount to allocate for new swapfile? (default: 4G) :128m

>> Step 2: File Name
>>>> Filename for new swapfile? (default: '/swapfile') :/swapfile_ext16

>>>> Continue? '/swapfile_ext16' (128M) will be created. (Y / N):y

>> 1. Creating swapfile...

# # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # #
Setting up swapspace version 1, size = 128 MiB (134213632 bytes)
no label, UUID=94e452f1-b252-4ea2-8d5f-e23634e34081
# # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # #

>> 2. Enabling swapfile...
>> 3. Swapfile added.

# # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # #
Current Swapfiles:

Filename                Type        Size    Used    Priority
/swapfile_ext3                          file        262140  238384  -5
/swapfile_ext7                          file        131068  106376  -3
/swapfile                               file        8388604 476148  -6
/swapfile_ext6                          file        131068  68840   -4
/swapfile_ext5                          file        131068  115304  -2
/dev/dm-1                               partition   1003516 0   -7
/swapfile_ext8                          file        131068  0   -8
/swapfile_ext9                          file        131068  0   -9
/swapfile_ext10                         file        131068  0   -10
/swapfile_ext11                         file        131068  0   -11
/swapfile_ext12                         file        131068  0   -12
/swapfile_ext13                         file        131068  0   -13
/swapfile_ext                           file        524284  0   -14
/swapfile_ext14                         file        131068  0   -15
/swapfile_ext15                         file        131068  0   -16
/swapfile_ext16                         file        131068  0   -17
# # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # #

>>>> Make swapfile permanent? (Y / N):y

>> 4. Created permanent swapfile. Modified '/etc/fstab'
>>>> Do you want to view '/etc/fstab?' (Y / N):y

# # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # #
# /etc/fstab: static file system information.
#
# Use 'blkid' to print the universally unique identifier for a
# device; this may be used with UUID= as a more robust way to name devices
# that works even if disks are added and removed. See fstab(5).
#
# <file system> <mount point>   <type>  <options>       <dump>  <pass>
/dev/mapper/ubuntu--vg-root /               ext4    errors=remount-ro 0       1
# /boot/efi was on /dev/nvme0n1p1 during installation
UUID=D6C3-F1E1  /boot/efi       vfat    umask=0077      0       1
/dev/mapper/ubuntu--vg-swap_1 none            swap    sw              0       0
/swapfile none swap sw 0 0
/swapfile_ext3 none swap sw 0 0
/swapfile_ext5 none swap sw 0 0
/swapfile_ext6 none swap sw 0 0
/swapfile_ext7 none swap sw 0 0
/swapfile_ext8 none swap sw 0 0
/swapfile_ext9 none swap sw 0 0
/swapfile_ext10 none swap sw 0 0
/swapfile_ext11 none swap sw 0 0
/swapfile_ext13 none swap sw 0 0
/swapfile_ext14 none swap sw 0 0
/swapfile_ext15 none swap sw 0 0
/swapfile_ext16 none swap sw 0 0
# # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # #
\$\endgroup\$
4
\$\begingroup\$

fallocate and swap files

You should not use fallocate for creating swap files. It's not supported.

From the mkswap manpage:

Note  that  a  swap  file  must  not contain any holes.  Using cp(1) to
create the file is not acceptable.  Neither is use of  fallocate(1)  on
file  systems  that support preallocated files, such as XFS or ext4, or
on copy-on-write filesystems like btrfs.   It  is  recommended  to  use
dd(1)  and  /dev/zero in these cases.  Please read notes from swapon(8)
before adding a swap file to copy-on-write filesystems.

And from the swapon manpage:

You should not use swapon on a file with holes.  This can  be  seen  in
the system log as

      swapon: swapfile has holes.

The  swap file implementation in the kernel expects to be able to write
to the file directly, without the assistance of the  filesystem.   This
is  a problem on preallocated files (e.g.  fallocate(1)) on filesystems
like XFS or ext4, and on copy-on-write filesystems like btrfs.

It follows that, while fallocate may be faster than dd, it's not suitable for creating swap files and not supported by swap-related tools.


# # # # # ... ?

You should limit this to the width of the display (or default to 80, which is pretty common as a default width for terminals). There's no reason to have it extend beyond the visible width (and possibly wrap around). You can obtain the width from the COLUMNS special variable, and get a substring:

bash-4.4$ echo "$COLUMNS"
66
bash-4.4$ line="${line:0:$COLUMNS}"
bash-4.4$ echo "$line"
# # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # #

Prompts for read

A echo -n "<prompt text>" before read is unnecessary, as read has the -p option:

 -p PROMPT  output the string PROMPT without a trailing newline before
    attempting to read

Case-insensitive case

Use the nocasematch option:

If the nocasematch shell option (see the description of shopt in The Shopt Builtin) is enabled, the match is performed without regard to the case of alphabetic characters.

So:

shopt -s nocasematch
case $yes_no in
    y|yes) ...
        ;;
    n|no|quit) ...

And so on.

This option also applies to the ==/!=/=~ tests in [[ ... ]].


The shopt -u extglob is unnecessary.


Exit after --help

It's fairly standard to have commands exit without doing anything if asked to print help instead of continuing on to act on other options. And if you plan to extend it for non-interactive usage, then you'll likely add support for options as well. So you may want to print help for all unsupported options, and for options which were incorrectly given. So a new function:

usage() {
    echo "$help_text"
    exit "${1:-0}"  # default exit status 0 if `$1` isn't given
}

An example option would look like:

case $1 in
   -n)
      [[ -z $2 ]] && usage 1 # error: need a second arg with `-n`, so exit 1
      do_something with "$2"
      ;;
   -h|--help)
      usage  # asked for help, exit 0
      ;;
   -*) # error: unrecognized option, so exit 1
      usage 1
      ;;
esac

(Of course, if you do add more options, you should look into getopts for parsing them.)


  1. Allow a non-interactive script mode that uses arguments for customizations
  2. Allow single/batch removal of swapfiles.
  3. Allow easier testing of each pathway of the interactive script

In that case, I'd strongly suggest that you extract out the actual actions (listing swap files, creating swap files, mounting them, etc.) into individual functions. This will allow you to mix and match actions based on options, allowing for easier testing and batch processing.

So the code might look like:

list_swapfiles() {
  printf "\n%s\n" "$line"
  printf "%s\n\n" "Current Swapfiles:"
  sudo swapon -s
  printf "\n%s\n" "$line"
}

# after parsing options
case $1 in)
  -l|--list)
    list_swapfiles
    exit
    ;;
  ...
esac

# no options, assume interactive
while true; do
  read -p ">>>> Are you sure you want to create a new swapfile? (Y / N):" yes_no
  case $yes_no in
    y|yes)       
      list_swapfiles
      break
      ;;
      ...
\$\endgroup\$
  • 1
    \$\begingroup\$ In addition to using the correct exit status, I'd also redirect usage to the standard error stream unless the help was requested: usage 1 >&2. \$\endgroup\$ – Toby Speight Aug 1 '18 at 11:54
  • \$\begingroup\$ @muru What is the scope of the shopt commands? If I use a shopt command then will it go back to the original usage after my script ends? Or does the life of that shopt command continue after my script ends? \$\endgroup\$ – Christopher Rucinski Aug 1 '18 at 12:11
  • 1
    \$\begingroup\$ @ChristopherRucinski the scope of the shopt built-in is the shell in which it was ran, which is the shell that was started to execute the script. See also superuser.com/a/176788/334516 \$\endgroup\$ – muru Aug 1 '18 at 17:19
  • \$\begingroup\$ @muru I was looking at implementing your suggestions for the read command, and I say that people suggest to always use read -r. Do you also suggest that? My code does not disallow back-slashes for filenames. \$\endgroup\$ – Christopher Rucinski Aug 1 '18 at 17:34
  • \$\begingroup\$ @ChristopherRucinski I didn't notice you were reading filenames using read. In that case, please use IFS= read -r. \$\endgroup\$ – muru Aug 1 '18 at 17:55
7
\$\begingroup\$

Portability

You mentioned portability as one of your top concerns. The commands used by the script limit it to certain systems:

  • swapon, mkswap: these are common in Linux systems
  • fallocate: available in Linux, supporting certain filesystems depending on the version
  • sudo: available in a wide range of systems but not always installed by default

This script is not portable, and I think that's not a real concern. It's designed to run in Linux systems with the supported filesystems. I think it wouldn't make sense to try to make it "portable".


In less specialized scripts that conceptually could make sense in many systems, using echo -e would be a portability concern. echo without flags works reliably, but its flags are not equally well supported in all systems. It's best to write scripts in a way to use only echo without any flags.

Broken help message

This is clearly a bug:

echo -e help_text

You probably intended this instead:

echo -e "$help_text"

And even then, the output would be pretty weird:

/n>> Documentation:



Usage: script.sh [-h]

Creates a new permanent swapfile


where:

    -h  displays help text

Probably this is not what you intended.

Readability

I find the script very hard to read. For example this:

help_text="/n>> Documentation:\n\n\t

Usage: $(basename "$0") [-h]\n\t 
Creates a new permanent swapfile\n\n\t
where:\n\t\t
    -h  displays help text"

Probably you intended something more like this:

help_text="
>> Documentation:

    Usage: $(basename "$0") [-h]
    Creates a new permanent swapfile

    where:
        -h  displays help text"

Which will output:

>> Documentation:

    Usage: a.sh [-h]
    Creates a new permanent swapfile

    where:
        -h  displays help text

There is no need for escape sequences like \n and \t, you can write them directly inside strings, a lot easier to read, and now you can echo "$help_text" without the -e flag and it just works.

Keep it simple

It's a good rule of thumb to keep things as simple as possible. For example you used the -e in all echo statements, even though most of them don't need it. I suggest to question the purpose of every single element, and cut out anything that isn't essential.


What is going on here?

?(-)@(-h|-help) )

Why not simply this:

-h|-help )

Some usability features can be simpler too. For example:

[Yy]|[Yy][Ee][Ss] )

How about like this instead:

[Yy]|yes|Yes )

The second is a lot easier to read. Sure, it's not the same thing, because the first version will match yEs too and the second won't. But is that really a problem?

Quoting

One of the most important things to learn properly in Bash scripting is how quoting works. It's not that complicated though. Whenever you use a variable as command arguments, they should be within double-quotes.

For example these statements are unsafe:

case $1 in

while $retry; do

They should be:

case "$1" in

while "$retry"; do

sudo in scripts

It's not great when a script needs to use sudo, because it can be annoying to get prompted for a password unexpectedly.

This script cannot do anything useful without sudo. It would be better to remove all the sudo from inside the script, and make it required to call the script itself with sudo. Add a simple check at the top of the script:

if [ "$(id -u)" != 0 ]; then
    echo "This script must be run as root or with sudo $0" >&2
    exit 1
fi

Check yourself

There is a nice online service where you can paste your shell script and it points out bugs and safety issues, called shellcheck.net.

\$\endgroup\$
  • \$\begingroup\$ Also, there is no exit 0 with the help case. The idea behind ?(-)@(-h|-help) is that will allow a user to type -h, --h, -help, or --help to get help. I have seen this double-hyphen usage for help quite a few times and wanted to emulate it somewhat. Do you think I should not do that? \$\endgroup\$ – Christopher Rucinski Jul 31 '18 at 8:35
  • 1
    \$\begingroup\$ @ChristopherRucinski wow. I didn't know it means that. It's not common to support both -help and --help. I suggest to stick to the GNU convention of -h and --help. That will be simpler. And simple is good. \$\endgroup\$ – Stop ongoing harm to Monica Jul 31 '18 at 8:57
  • \$\begingroup\$ I agree. I recently found that the convention for double-dash is to use a word or phrase (--help) and the single-dash is for a letter (-h). With that it is uncommon to see --h and -help. I didn't know that, so I will use your suggestion \$\endgroup\$ – Christopher Rucinski Jul 31 '18 at 15:09
  • \$\begingroup\$ can you please provide resources I can review for your comment Whenever you use a variable as command arguments, they should be within double-quotes. I don't understand when I should and shouldn't double-quote variables, and I don't fully understand why they are unsafe, yet. \$\endgroup\$ – Christopher Rucinski Jul 31 '18 at 15:28
  • 1
    \$\begingroup\$ @muru that came from the manpage on my system, indeed that was too specific to mention without the source. Thanks, updated. \$\endgroup\$ – Stop ongoing harm to Monica Aug 1 '18 at 5:35

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.