Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

The following script closes a virtual machine (which is run in a screen session), waits for the session to close, creates a backup of the VM, and restarts the VM. The shutdown and bootup scripts speak for themselves, but I can post them if necessary. Is there any way to clean up the sockets_found function? It seems like there should be an easier way to detect whether or not screen has any open sessions.

#!/bin/bash

now=`date '+%Y%m%d'`

# No Sockets found
# There is a screen on
function sockets_found {
    screen -ls | grep "There is a screen on"
    if [ $? -eq 1 ]; then
        return 1
    else
        return 0
    fi
}

function wait_for_sockets_to_close {
    while sockets_found; do
        echo "Waiting for screen to close..."
        sleep 1
    done;
}

echo "Shutdown VM..."
/bin/bash ~/shutdown.sh
wait_for_sockets_to_close
# ensure that the backup directory exists
mkdir -p ~/backup
echo "Copying VM to backup directory..."
cp -Rf ~/VirtualBox\ VMs/ ~/backup/VirtualBox\ VMs${now}/
echo "Booting VM..."
/bin/bash ~/bootup.sh
share|improve this question
up vote 1 down vote accepted

This can be improved and simplified:

function sockets_found {
    screen -ls | grep "There is a screen on"
    if [ $? -eq 1 ]; then
        return 1
    else
        return 0
    fi
}

Like this:

function sockets_found {
    screen -ls | grep -q "There is a screen on"
}

That is:

  • You can remove the if statement, as the exit code of grep naturally becomes the exit code of the function
  • If the exit code of grep is greater than 1, the original function exits with 0. That's inappropriate. Such exit codes indicate errors in grep, and do not imply that screen sessions exist
  • I added the -q flag to suppress the output of grep in case of success

Other minor things:

  • No need for the ; in done;
  • It would be better to double-quote ${now} in the backup target directory to prevent accidental globbing or word splitting, in case you might make a mistake in the syntax of the date command
  • It would be good to add some blank lines in the last chunk of commands for better readability

Like this:

echo "Shutdown VM..."
/bin/bash ~/shutdown.sh

wait_for_sockets_to_close

# ensure that the backup directory exists
mkdir -p ~/backup

echo "Copying VM to backup directory..."
now=$(date '+%Y%m%d')
cp -Rf ~/VirtualBox\ VMs/ ~/backup/VirtualBox\ VMs"${now}"/

echo "Booting VM..."
/bin/bash ~/bootup.sh
share|improve this answer

You also can find if the socket file exists on /var/run/screen/S-yourname/pid.screenname.

share|improve this answer

There is no need to grep here:

screen -ls | grep "There is a screen on"
if [ $? -eq 1 ]; then

Screen -ls will return with exit code 1 in the case no sockets are found in /var/run.

I would run the date command just before the copy is run, in case the sockets close during a date roll and you get the wrong date on your backup.

echo "Copying VM to backup directory..."
now=$(date '+%Y%m%d')
cp -Rf ~/VirtualBox\ VMs/ ~/backup/VirtualBox\ VMs${now}/

As above use $(cmd) notation instead of backtick notation. They achieve the same thing, but $() is visually clearer and can be nested.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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