[SOLVED] [BASH]first script, suggestions for improving!

RaptorX · 08-26-2009, 04:22 PM

Hi guys!

While following the Advance Bash Scripting tutorial I decided to exercise a little bit and I just finished my first complete bash script.

I would like to hear your opinion on what to improve, be it methodology, commands to use, or whatever comes to mind...

This small script came to mind because I needed to mount multiple images at once and then I decided to expand my little for loop that i had sticked in a file a while ago...

here is the code:

Code:

#!/bin/bash
# mountsc.sh
# --------------
# This script depends on which name you use to call it.
# mountemall - shortcut to mount all images at once
# umountemall - shortcut to umount all the images and delete temporal files.

IMG_DIR=$1
MOUNT_DIR=/mnt
TEMP_FOLDER=$MOUNT_DIR/tmp

if [ "$1" = '-h' -o "$1" = '--help' ]; then

cat <<MOUNTSC
>>>>>-----------------------------------------------------------------------<<<<<<

This script will create a temporal folder in /mnt and will  mount/unmount several
images at the same time,from the specified folder.

If no folder is specified it will use the current directory instead.

Usage:
mountemall  [path/to/images]
umountemall [path/to/images]

Assuming that the path /home/movies/ contains several dvd images you can do:

mountemall  /home/movies
umountemall /home/movies

Must be root or sudoer to execute this script.

>>>>>-----------------------------------------------------------------------<<<<<<
                          --Created by RaptorX--
                              26th-Aug-2009

MOUNTSC

exit 0

fi

mkloopdevs() { # Make loop devices

        for dev in `seq 10 20`; do mknod /dev/loop$dev b 7 $dev; done
} # adds 10 loop devices, adjust to your needs

mktempdirs() { # Make temporal directories

        for dir in `ls -X1 $IMG_DIR`; do
                mkdir -vp $TEMP_FOLDER/temp-$dir
        done
} #  this is where the images will be mounted, this folders will be deleted
  #+ by the umountemall command.

deltempdirs(){ # delete temporal directories

        rm -rfv $TEMP_FOLDER
} # this is the function responsible of deleting the temporal folders.

if [ $0 = "/usr/local/bin/mountemall" ]; then

        if [ -z $1 ]; then
                IMG_DIR=$PWD
                echo; echo "Directory not specified...using current directory"; echo;
        fi
        echo "Creating additional loop devices"
        echo ">>--------------------------<<"
        mkloopdevs 2> /dev/null
        let count=0
        echo; echo "Creating temporal directories"
        echo ">>--------------------------<<"
        mktempdirs
        echo ">>--------------------------<<"
        echo; echo "Mounting all images in $IMG_DIR"
        echo ">>--------------------------<<"
        for img in `ls -X1 $IMG_DIR`; do
                mount $IMG_DIR/$img $TEMP_FOLDER/temp-$img -o loop
                let count=$count+1
        done
        mount | grep loop
        echo "$count files where successfuly mounted in $TEMP_FOLDER"
        exit 0
fi

if [ "$0" = "/usr/local/bin/umountemall" ]; then
        echo "Unmounting all loop devices and Deleting temporal folders"; echo
        umount /dev/loop* 2> /dev/null
        deltempdirs
        exit 0
fi

Thanks for your support!

colucix · 08-26-2009, 04:35 PM

Nice! I just have a little note: you've placed the script in a directory that is usually included in every user's PATH: in this way whenever the script is executed, the shell look for it in $PATH and runs the script with its full path /usr/local/bin/mountemall. This results in a correct substitution for $0. However there is a chance that a user cd to /usr/local/bin and issues the command

Code:

./mountemall

this time $0 is expanded to ./mountemall and the if condition is not matched anymore. To avoid this behaviour you can change $0 with

Code:

$(readlink -f $0)

this is a method to obtain the full path of the script independently from the actual command issued. I think it is more robust this way. Just my

RaptorX · 08-26-2009, 04:38 PM

@culocix

I didnt even asked for it and you just answered it...
That was bothering me when i was writing the code but thought that if I continued reading I could find a solution for it...

that was a great help!

GrapefruiTgirl · 08-26-2009, 05:07 PM

All in all, for your first script, I say WAY TO GO!

Here's a few suggestions. Not really suggestions, but rather, the way *I* would do a couple things -- though there's nothing "wrong" with the way you've done it -- but as you learned in your other [bash] thread, there are many ways of doing things. So, I like to sort of 'compact' my code a little:

I would change this:

Code:

if [ "$1" = '-h' -o "$1" = '--help' ]; then

# <show help information, then exit>
exit 0

fi

to this:

Code:

case $1 in -h|--help)
 # <show help information, then exit>
 exit 0 ;;
esac

and.. when executing stuff in `backticks` this spawns a sub-shell, which in itself isn't a bad thing, but to save a tiny bit of resources, you could use $(this method) instead. It has another advantage too: you will eventually discover yourself needing multiple embedded instances of sub-shell backticked code, and once you get one or more embedded `backticked` arguments, and throw in an echo command or something, the code not only gets confusing, but also starts to act weird, because the shell has trouble interpreting the embedded backticks. So, I would change things like this:

Code:

`seq 10 20`

to this:

Code:

$(seq 10 20)

It may seem insignificant, but believe me, it will simplify *something* down the road for you.

Now, this here:

Code:

mkloopdevs 2> /dev/null

that looks like it tosses any error information right into the abyss

which may not be what you want, if an error occurs. There are ways of error checking and/or sanity checking, but I'll leave that to you to research for the time being.

Now... The line below, you aren't checking for any result from the grep. I realize that you just want to show what loop items exist, but if there are none, it would indicate a problem. Again, this falls into the 'error checking' department.

Code:

mount | grep loop

One final thing, which I *think* colucix just addressed, is the absolute vs relative path of the script. When referring to itself in the HELP information, try changing:

this:

Code:

Usage:
mountemall [path/to/images]
umountemall [path/to/images]

to this (lol, can't remember if we need quotes around the variables):

Code:

Usage:
$0 mountemall [path/to/images]
$0 umountemall [path/to/images]

so it will show the whole path.

Anyhow, again: excellent work! Many ways to do stuff, but I think your script looks like it will work just fine

though I haven't actually tried to execute it.

Sasha

jschiwal · 08-26-2009, 06:52 PM

Some random nitpicking ideas for your consideration.

You could use "losetup -fs" to grab the next available loop device and return the one that was used.

for dir in `ls -X1 $IMG_DIR`

You can use
for imagefile in "${IMG_DIR}"/*.iso
or
for imagefile in "${IMG_DIR}"/*

Using `ls' isn't necessary for your example, but read on. Maybe you want to use find instead.

Requiring an extension might be a good idea for when the current directory is used.
Then you could use
mkdir "${imagefile%.*}" creating a directory with the same base name, without the extension. Otherwise you will be trying to create a directory with the same name as the file when your mount dir and image dir are the same. You could instead alter the name as in "_${imagefile}" or append a random number at the end. Look in the /tmp/ directory. You will see many examples of this.

You could instead use `find' instead of ls, so that you ignore directories:
for imagefile in $(find ${IMG_DIR} -maxdepth 1 -type f); do

mount $IMG_DIR/$img $TEMP_FOLDER/temp-$img -o loop
The filetype isn't given. This might cause the mount command to fail.

Consider piping the list of image file to the file command and rejecting ones of type DATA, and parsing the filetype from the results. You might want to use a temporary file for this.

Also be careful for cases where a filename contains spaces or other evil characters. You need to develop the habit of enclosing variables in double quotes. E.G. "temp-${img}"

Another common technique is using the -print0 option for the `find' command, and piping the list of filenames to `xargs -0':

cd ${MOUNT_DIR}
find "${IMG_DIR}" -type f -print0 | xargs -0 mkdir

If you are processing the list of file, or reading from a file, you can use the `tr' command to convert returns to nulls:
cat filelist | tr '\n' '\0' | xargs -0 file | sed -f filterimages.sed >/tmp/imagefilelist

Another thing to consider is when the image file is an image of a disk and not of a partition. The file command may be able to help there to, telling you that it is a bootable image and give the offsets to the partitions. Maybe this isn't what you intend for your script.

One of the best things you did in your script is to use functions to keep it modular in design. Doing so will make it easier for you to make changes later on.

By the way, before I forget. Good job!

catkin · 08-27-2009, 09:37 AM

Hello RaptorX

Quote:

Originally Posted by RaptorX

I would like to hear your opinion on what to improve, be it methodology, commands to use, or whatever comes to mind...

Nice bit of scripting

Here goes with some comments for your consideration ...

Quote:

Originally Posted by RaptorX

Code:

#!/bin/bash
# mountsc.sh
# --------------
# This script depends on which name you use to call it.
# mountemall - shortcut to mount all images at once
# umountemall - shortcut to umount all the images and delete temporal files.

A couple of things you might like to add to that header: usage instructions and change history. Usage can be moved into a function which you could reference in the header, or you could simply advise how to run the script and get usage instructions. Here's an example

Code:

# Usage: 
#	See usage function below (search for "function usage") or use -h option.

Quote:

Originally Posted by RaptorX

Code:

IMG_DIR=$1
MOUNT_DIR=/mnt
TEMP_FOLDER=$MOUNT_DIR/tmp

Generally squeaking it's good defensive programming to put all variable references in double quotes so any whitespace in their values does not cause (as many, or as mysterious) surprises. In similar vein, "set -o nounset" defends against mis-typed variable names and unset variables.

A common convention is to use uppercase names for environment variables and lowercase for the rest. A personal taste thing.

Quote:

Originally Posted by RaptorX

Code:

if [ "$1" = '-h' -o "$1" = '--help' ]; then

The [[ ]] form of test is more robust than [ ], in the same way that $( ) is more robust than ` ` as Sasha already pointed out (but I understand both create sub-shells).

Quote:

Originally Posted by RaptorX

Code:

This script will create a temporal folder in /mnt

Not exactly a scripting matter but "temporal" has several meanings. If someone else may maintain this script then "temporary" would be clearer. Also not a scripting matter but how about making the temporary directory in /tmp? That way it would automatically be cleaned up at reboot (distro-dependent?) in case something goes wrong.

Quote:

Originally Posted by RaptorX

Code:

mkloopdevs() { # Make loop devices

The alternative form "function mkloopdevs {" makes it easier to search for functions -- helpful if the script grows large.

Quote:

Originally Posted by RaptorX

Code:

for dev in `seq 10 20`; do mknod /dev/loop$dev b 7 $dev; done

Safer to give full path on all commands, for example /bin/mknod in this case. That guards against unexpectedly picking up an alias instead of the command itself or of either not finding the command or finding a different command (when $PATH is not as expected). This is especially important for security when running with root privileges.

Error trapping helps identify trouble and avoid unintentional actions. There are almost no commands that should not be error trapped! Here's an idiom for command error trapping

Code:

buf="$( command 2>&1 )"
rc=$?
if [[ $rc -ne 0 -o "$buf" != "" ]]; then
   echo "Unexpected output when <doing something>. Return code was $rc. Output was: '$buf'"
   exit 1
fi

Alternatively, if the command is expected to produce output:

Code:

buf="$( command 2>&1 )"
rc=$?
case "$buf" in
    <expected output 1> | <expected output 2> | ... )
        ;;
    * )
        echo "Unexpected output when <doing something>. Return code was $rc. Output was: '$buf'"       
esac

This requires * in the expected output patterns if the output includes variable text.

Quote:

Originally Posted by RaptorX

Code:

mktempdirs() { # Make temporal directories

for dir in `ls -X1 $IMG_DIR`; do
mkdir -vp $TEMP_FOLDER/temp-$dir
done
} # this is where the images will be mounted, this folders will be deleted
#+ by the umountemall command.

Indentation makes it easier to see that "{-}"s, "do-done"s, "if-fi"s etc are correctly matched and easier to scan down the code to identify the block of interest. Not an issue when the do-done is a short as this but becomes important with larger and more complex code blocks. Here's the same code indented

Code:

mktempdirs() { # Make temporal directories

    for dir in `ls -X1 $IMG_DIR`
    do
        mkdir -vp $TEMP_FOLDER/temp-$dir
    done
} # this is where the images will be mounted, this folders will be deleted
  #+ by the umountemall command.

Best

Charles

RaptorX · 08-27-2009, 11:34 AM

Thanks for all your replays!
This post will be a little bit long because I will try to answer to everybody and you all provided a LOT of info so... bear with me or jump to your nick and see your specific answer.

First I have to specify and remind you all that I am totally new to bash scripting and that error parsing, data filtering and other things which i consider little bit advanced for THIS particular code will be added in the future when i arrive to those chapters and really understand what I am doing...

I can say that I am proud that I understand this code 100% but to tell the truth if I start copy pasting your codes I would be cheating myself because some of them I simply cat get it.

That said, I have to say that you actually cleared a lot of questions that I had and this was exactly what I wanted to see.

Now let me turn on the -vv option and answer in detail:

@RaisinGirl
#have you noticed that I always change your nick?? I never remember your nick...

I did not use "case" because I havent arrive to that command in the book yet and to tell the truth i am not 100% sure how it works since the "case" command and equivalents in other languages have a very different syntax, but it sure looks more tidy, and I believe that it is easier to keep track of when the code is buggy.

I thought that the $() also spawned a new shell... also () does that but {} does not.

Code:

mkloopdevs 2> /dev/null

Thanks for pointing it out, I actually put that in there and in other places just for debugging and I forgot to erase it... the thing is that I was checking what happened when i ran the script within a folder that contained other files besides images but I knew the error that was going to be in stdout so I simply put it away.

Quote:

to this (lol, can't remember if we need quotes around the variables):
Code:

Usage:
$0 mountemall [path/to/images]
$0 umountemall [path/to/images]

As far as I read the quotes are for making sure spaces or other weird characters in the variable expansions dont create any problem... Thanks for this suggestion and I think that:

Quote:

$0 [path/to/img]

will work perfectly since $0 should print the current directory.

and Thanks!

@jschiwal

Quote:

You could use "losetup -fs" to grab the next available loop device and return the one that was used.

I was already wondering how to do that, but I was not familiar with the "losetup" command (today i will read the man pages of it and readlink which is also new for my toolkit). Thanks!

Quote:

Requiring an extension might be a good idea for when the current directory is used.
Then you could use
mkdir "${imagefile%.*}" creating a directory with the same base name, without the extension. Otherwise you will be trying to create a directory with the same name as the file when your mount dir and image dir are the same. You could instead alter the name as in "_${imagefile}" or append a random number at the end. Look in the /tmp/ directory. You will see many examples of this.

You could instead use `find' instead of ls, so that you ignore directories:
for imagefile in $(find ${IMG_DIR} -maxdepth 1 -type f); do

mount $IMG_DIR/$img $TEMP_FOLDER/temp-$img -o loop
The filetype isn't given. This might cause the mount command to fail.

It is true what you said, when i run my command it creates folders with the exact name of the file (including the extension) but now i think that mkdir "${imagefile%.*}" should take care of that.

I did not specify the type of file since the images can be of ANY type and I do not know (or want to spend time) adding "if's" or "case's" for each type. And of course, experience... I lack of it...

Quote:

Consider piping the list of image file to the file command and rejecting ones of type DATA, and parsing the filetype from the results. You might want to use a temporary file for this.

Can you use --verbose mode in this one?? I am really interested!

I was trying to mount a folder that contained a bunch of images and some other type of files and as expected my script also tried to mount the other files, which is not desired but it was expected since I dont know how to filter image files independent of their type and extension! I have some *.iso files renamed to *.img, what I want is that my script distinguish ANY mountable file even though it might have different extensions or even no extension at all!

Quote:

Also be careful for cases where a filename contains spaces or other evil characters. You need to develop the habit of enclosing variables in double quotes. E.G. "temp-${img}"

Thanks for the hint~! I will keep it in mind!

@catkin

Quote:

Generally squeaking it's good defensive programming to put all variable references in double quotes so any whitespace in their values does not cause (as many, or as mysterious) surprises. In similar vein, "set -o nounset" defends against mis-typed variable names and unset variables.

It makes sense... I will do it from now on!
Now, can you explain me the use of set -o nounset? you said something there but my brain didnt quite catch it.

Quote:

A common convention is to use uppercase names for environment variables and lowercase for the rest.

Interesting, so it is better if I set the variables at the beginning in lowercase? (for the sake of convention)

Quote:

Not exactly a scripting matter but "temporal" has several meanings. If someone else may maintain this script then "temporary" would be clearer. Also not a scripting matter but how about making the temporary directory in /tmp? That way it would automatically be cleaned up at reboot (distro-dependent?) in case something goes wrong.

Well you are actually right, temporary is more accurate in this case.

About the directory where to mount it, I was thinking that it was more logical to expect that something that was just "mounted" should be in the /mnt directory but thats exactly why I set the mount folder as a variable since other people would rather have that in other places, then you simply change that variable... I am also thinking maybe adding the option to allow you to choose a mount directory when executing the script,which might be more "user friendly"....but I really dont like "user friendly" stuff since I think that it tends to "stupify" the users who use it...

Thanks for the other pointes as well, I will keep them in mind!

So guys!
As I said before.... thanks for your support!
I will post an updated code to see more or less how it looks later on!
I learned a lot!

catkin · 08-27-2009, 12:54 PM

Quote:

Originally Posted by RaptorX

I thought that the $() also spawned a new shell... also () does that but {} does not.

That's right.

Quote:

Originally Posted by RaptorX

I think that:

Code:

$0 [path/to/img]

will work perfectly since $0 should print the current directory.

It will, well -- the name with which the script was called. You meant "including the current directory" as in ./mountemall? Because this is in a help message you probably don't want any pathname so

Code:

${0##*/} [path/to/img]

would be prettier (it strips everything up to the last "/")

Quote:

Originally Posted by RaptorX

I did not specify the type of file since the images can be of ANY type and I do not know (or want to spend time) adding "if's" or "case's" for each type. And of course, experience... I lack of it...

Can you use --verbose mode in this one?? I am really interested!

I was trying to mount a folder that contained a bunch of images and some other type of files and as expected my script also tried to mount the other files, which is not desired but it was expected since I dont know how to filter image files independent of their type and extension! I have some *.iso files renamed to *.img, what I want is that my script distinguish ANY mountable file even though it might have different extensions or even no extension at all!

Something like this (time to explore the case command!)

Code:

for file in <possible image files>
do
    case "$(/usr/bin/file "$file")"
        *'JPEG'* | *'PNG'* | *'XCF'* )  # Add others as required
            <commands to do what you want with the image file>
            ;;
    esac
done

In this code the { } doesn't do anything

Quote:

Originally Posted by RaptorX

E.G. "temp-${img}"

It's harmless but ineffectual. { } is only required when doing parameter expansion, when referencing an array element or when the following character could be part of the variable name, such as in

Code:

animal='cat'
echo "The ${animal}s were wild"

Quote:

Originally Posted by RaptorX

Now, can you explain me the use of set -o nounset? you said something there but my brain didnt quite catch it.

If you put

Code:

set -o nounset

at the beginning of the script then any time your script encounters a variable which you have not set, bash generates an error. This helps during development in case you have mis-typed a variable name or if the variable has not been set. It can also help in production if changes in the data being processed changes the way the script works.

But! This is not the behaviour you want from

Code:

if [ -z $1 ]; then

because when the script is called with no arguments bash will write "bash: $1: unbound variable". In these cases, where you know the variable may be unset, you can use ${1:-}. It sets the variable to empty if it is not set and so avoids the problem.

Quote:

Originally Posted by RaptorX

About the directory where to mount it, I was thinking that it was more logical to expect that something that was just "mounted" should be in the /mnt directory but thats exactly why I set the mount folder as a variable since other people would rather have that in other places, then you simply change that variable...

Good technique!

Quote:

Originally Posted by RaptorX

I am also thinking maybe adding the option to allow you to choose a mount directory when executing the script,which might be more "user friendly"....but I really dont like "user friendly" stuff since I think that it tends to "stupify" the users who use it...

Always a balance between the elegantly minimal and the feature laden ...

RaptorX · 08-28-2009, 05:53 PM

@catkin

The kind of "images" that i am using is like "iso" or "img"

not pictures!
but I will start using the case command later on when I arrive to that chapter.

RaptorX · 08-29-2009, 05:11 PM

I was playing with losetup and I am not sure how can I use it in my script since losetup -f just tells me which existing loop device is free, but in most distros you only have 7 loop devs and what i want to do is to create devices as needed...

The indentation on my first code was not working because i quoted the text by mistake instead of wrapping it in code tags...