5
\$\begingroup\$

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 
\$\endgroup\$

    3 Answers 3

    1
    \$\begingroup\$

    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 
    \$\endgroup\$
      2
      \$\begingroup\$

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

      \$\endgroup\$
        1
        \$\begingroup\$

        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.

        \$\endgroup\$

          Start asking to get answers

          Find the answer to your question by asking.

          Ask question

          Explore related questions

          See similar questions with these tags.