Skip to main content
replaced http://unix.stackexchange.com/ with https://unix.stackexchange.com/
Source Link
  • You might only present USB block devices as options (Look for /dev/disk/by-path/*-usb-*.)
  • You could look for a device with a particular volume label. (Look in /dev/disk/by-label/NAME.)
  • You could check whether the device is on removable mediaremovable media.
  • You might only present USB block devices as options (Look for /dev/disk/by-path/*-usb-*.)
  • You could look for a device with a particular volume label. (Look in /dev/disk/by-label/NAME.)
  • You could check whether the device is on removable media.
  • You might only present USB block devices as options (Look for /dev/disk/by-path/*-usb-*.)
  • You could look for a device with a particular volume label. (Look in /dev/disk/by-label/NAME.)
  • You could check whether the device is on removable media.
Source Link
200_success
  • 145.7k
  • 22
  • 191
  • 481

I assume you are running this script on Linux.

Prompting the user for a device to mount and unmount is a bit unusual, not to mention user unfriendly and error prone. What is your intention — to find a USB memory stick? If so, you could mitigate some of the danger by limiting the user's choices.

  • You might only present USB block devices as options (Look for /dev/disk/by-path/*-usb-*.)
  • You could look for a device with a particular volume label. (Look in /dev/disk/by-label/NAME.)
  • You could check whether the device is on removable media.

Here's one way to list removable media and their partitions:

for blkdev in /sys/block/* ; do
    if [ "`cat $blkdev/removable`" = 1 ]; then
        for devnum in `cat "$blkdev"/dev "$blkdev"/*/dev 2>/dev/null` ; do
            for node in /dev/* ; do
                [ "`stat -c %t:%T "$node"`" = "$devnum" ] && echo "$node"
            done
        done
    fi
done

Instead of mounting the device at /media, I suggest creating a temporary directory with mktemp -d and mounting it there. Linux desktop environments often use subdirectories of /media as places where devices are automatically mounted on insertion.


Restarting the script with sh "$myself" is problematic in several ways:

  • You've hard-coded the name of the script, so it will break if anyone renames the script. You could use $0 instead.
  • You've assumed that auto_rsa_gen.sh is in the current directory.
  • The first run is done in Bash due to the shebang line, but subsequent runs are in sh (usually Bash in Bourne-shell compatibility mode).
  • You didn't use exec, so execution of the outer script will continue with the wrong device after the inner script completes successfully with the right device.

The straightforward solution, which is to use a while loop, would have avoided all of those pitfalls.


The validation of $ident does not happen after the prompt, but is interrupted by the prompt for $cnum. That's confusing to the user and to the programmer.


You should echo the confirmation of copying within the loop:

cp "$f" /media/ && echo "Copied $f to /media" \
                || echo "Failed to copy $f to /media"

This is better because:

  • You avoid running find twice, which is more efficient.
  • You avoid running find twice, which is more maintainable.
  • The confirmation message only appears if copying succeeded.
  • It acts as a progress indicator.

It is idiomatic to put the do at the end of the same line as while and for, like this:

while condition ; do
    stuff
done

for i in $list ; do
    stuff_with "$i"
done

Similarly, then follows on the same line as if:

if $condition ; then
    something
else
    something_else
fi

Your indentation will make more sense if you follow these conventions.