Help with improving korn shell script

Shell Programming and Scripting
1

I am primarily a SQA/Tester and new to korn shell. How can I improve the following script?

#/bin/ksh
SourceLocation=~/Scripts/Test/Source
TrackerLocation=~/Scripts/Test/Tracker
TargetLocation=rdbusse@rdbmbp:/Users/rdbusse/Scripts/Test/Target

for file in $(cd $SourceLocation; ls)
do

  echo Step 1: Examine $file
  if [ ! -f $TrackerLocation/$file ];
  then
    echo Step 2: Processing $file
    eval scp -p $SourceLocation/$file $TargetLocation/$file
    ret_code=$?
    if [ $ret_code != 0 ]; then
      printf "Error : [%d] when executing command: $ret_code
      #exit $ret_code
    else
      echo Step 3: Writing $TrackerLocation/$file
      touch $TrackerLocation/$file
    fi
  else
    echo Step 2: $file previously processed
  fi

done
2

Please use code tags as required by forum rules!

When asking to improve something, it would be nice to supply

  • the objective (although quite clear in this case)
  • the problem(s) (malfunctions, error msgs, performance issues, ...)
3

Thanks RudiC,

The scripts lists and transfers files from the Source to the Target and then Logs the process states within the Tracker. I have run the script successfully, I am just asking for suggestions in improving the script. How enhance certain statements, or what other commands could do a more efficient operation.

4

You could try using the rsync utility:

#/bin/ksh
SourceLocation=~/Scripts/Test/Source
TargetLocation=rdbusse@rdbmbp:Scripts/Test/Target

rsync -av --del $SourceLocation/ $TargetLocation

This syncs all files under $SourceLocation to $TargetLocation (including sub-directories) and also removes any extra files or directories in Target but not in Source.

1 Like
5

Does the rsync utility replace the rest of the code i.e. the loop code?

6

yes It has a number of options to control how it works and will usually examine the date/time/crc of existing files and only synchronize anything that is different on the target system.

This may be useful if you would like any updates to scripts to also be applied to the target system.

7

If you just want some improvements to the existing script, I would recommend:

#!/bin/ksh
SourceLocation=~/Scripts/Test/Source
TrackerLocation=~/Scripts/Test/Tracker
TargetLocation=rdbusse@rdbmbp:/Users/rdbusse/Scripts/Test/Target

for sfile in $SourceLocation/*
do
  [ -f "$sfile" ] || continue
  file=${sfile##*/}

  echo Step 1: Examine $file
  if [ ! -f "$TrackerLocation/$file" ];
  then
    echo Step 2: Processing $file
    scp -p "$SourceLocation/$file" "$TargetLocation/$file"
    ret_code=$?
    if [ $ret_code != 0 ]; then
      printf "Error : [%d] when executing command:" $ret_code
    else
      echo Step 3: Writing $TrackerLocation/$file
      touch "$TrackerLocation/$file"
    fi
  else
    echo Step 2: $file previously processed
  fi

done

Improvements:
Use shell to expand $SourceLocation/* not a sub-shell + ls command.
Ignore sub-directories or other special files within the script directory.
Quoted $file references to cater for white space in file names.
Avoid unnecessary eval call for security reasons.

8

Let me address a few things, not necessarily about optimization, but rather basics of scripting in the shell.

#!/bin/ksh

missing highlighted.

Try to double quote variable deference. That has some benefits.

$(cd $SourceLocation; ls)

ls is an interactive tool that most of the times is not necessary for scripting. ls is specially not an appropriated way to feed a for loop, due to the many ways it can break your code.
Separate the cd command from the loop. Do a cd first, verify that you are in the directory you want (in case that the variable does not contain an existing directory, it will cd into your home).
file is already a command, try to avoid it as a variable as a manner of clarity.

cd "$SourceLocation"
if [ "$PWD" != "$SourceLocation" ]; then
    echo "$PWD is not the directory you want, bailing out"
    exit
fi

for filename in *; do
     ...
     if [ ! -f "$filename" ]; then
         ...
     fi
     ...
done

Could substitute your for loop. However, if the shell can not expand to any files, the for loop will happily pass the string "*" along the way, and your logic will process it.

1 Like