2
\$\begingroup\$

I'm working on a script that will archive, compress, clear old archive files, then remove the source file if it is below a set size. The output when I tested it worked, but I just wanted a second look to make sure I'm not missing any issues.

#!/usr/bin/ksh
# Script Name: archiving.ksh
# Purpose    : To copy and  Archive the files
#              $1 = Source file name pattern with fully qualified path
#-------------------------------------------------------------------------------------------------
#set -xv

sourcedir=$1
filename_pattern=$2
arcdir=/busdata/data/archive
filename=`ls $sourcedir/$filename_pattern*`
file_basename=`basename $filename`

echo $sourcedir
echo $arcdir
echo $filename_pattern
echo $filename
echo $file_basename

echo "Copying $filename to ${arcdir}"
cp $filename ${arcdir}/${file_basename}_$(date +%Y%m%d%H%M%S)
compress ${arcdir}/$file_basename*

echo "Removing files that are more than 30 days old from ${arcdir}"
find ${arcdir} -mtime +15 -type f -exec rm -f {} \;

echo "Remove empty file"
find ${sourcedir} . -name "858_file_*.exp" -type 'f' -size -160k -exec rm -f {} \;
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Is this intended to be portable (POSIX) shell? And have you run shellcheck on it? \$\endgroup\$ Nov 15, 2022 at 21:31
  • 1
    \$\begingroup\$ I have not run a shellcheck on it. it seems when I copied it over the top don't make it \$\endgroup\$ Nov 15, 2022 at 23:09

2 Answers 2

1
\$\begingroup\$

I would make the following changes to the script for clarity and ease of use by others:

#!/usr/bin/ksh
# Script Name: archiving.ksh
# Purpose    : To copy and archive the specified files
#              $1 = Fully qualified source directory for the script to run from
#              $2 = Source file name pattern
#-------------------------------------------------------------------------------------------------
# set -xv

if [ "$#" -ne 2 ]; then
  echo >&2 "Illegal number of parameters passed to the script!"
  exit 1
fi

sourcedir="$1"
filename_pattern="$2"
arcdir="/busdata/data/archive"
filename=`ls $sourcedir/$filename_pattern*`
file_basename=`basename $filename`

echo "$sourcedir"
echo "$arcdir"
echo "$filename_pattern"
echo "$filename"
echo "$file_basename"

echo "Copying $filename to ${arcdir}..."
cp "$filename" "${arcdir}/${file_basename}_$(date +%Y%m%d%H%M%S)"
compress "${arcdir}/$file_basename"*

echo "Removing files that are more than 30 days old from ${arcdir}..."
find "${arcdir}" -mtime +15 -type f -exec rm -f {} \;

echo "Removing empty files..."
find "${sourcedir}" . -name "858_file_*.exp" -type 'f' -size -160k -exec rm -f {} \;

The first change is enforcing the expected number of script parameters, so the code doesn't behave unexpectedly if fewer parameters are given. The syntax is explained here if you are unfamiliar.

The next step is quoting all shell variables wherever possible for type safety, it's considered a best practice when you aren't sure what values will be passed. More information about this can be found in this thread.

Outside of these changes I don't see anything that throws up any red flags in your code, it looks like it will do what is expected from it and nothing more/less.

\$\endgroup\$
0
0
\$\begingroup\$

Firstly, the header comment is inaccurate, as it mentions only one argument, but the script expects two.

As Aaron says, quote all expansions, since you don't want word splitting to be performed on any of them. That's the majority of the shellcheck results:


281239.sh:11:10: note: Use $(...) notation instead of legacy backticks `...`. [SC2006]
281239.sh:11:14: note: Double quote to prevent globbing and word splitting. [SC2086]
281239.sh:11:25: note: Double quote to prevent globbing and word splitting. [SC2086]
281239.sh:12:15: note: Use $(...) notation instead of legacy backticks `...`. [SC2006]
281239.sh:12:25: note: Double quote to prevent globbing and word splitting. [SC2086]
281239.sh:14:6: note: Double quote to prevent globbing and word splitting. [SC2086]
281239.sh:16:6: note: Double quote to prevent globbing and word splitting. [SC2086]
281239.sh:17:6: note: Double quote to prevent globbing and word splitting. [SC2086]
281239.sh:18:6: note: Double quote to prevent globbing and word splitting. [SC2086]
281239.sh:21:4: note: Double quote to prevent globbing and word splitting. [SC2086]
281239.sh:21:24: note: Double quote to prevent globbing and word splitting. [SC2086]
281239.sh:21:41: warning: Quote this to prevent word splitting. [SC2046]
281239.sh:22:20: note: Double quote to prevent globbing and word splitting. [SC2086]
281239.sh:28:6: note: Double quote to prevent globbing and word splitting. [SC2086]

A lot of expansions use braces where not necessary - I find that to be visual clutter, but if you prefer it, at least be consistent and use them everywhere.

Avoid substituting the output of ls into variables - many versions of ls will transform filenames for human readability (e.g. showing control characters). And if the pattern expands to multiple filenames, file_basename will end up being something that's not usable in the way we attempt here.

I think the program is much too chatty - consider providing an option to turn on debugging output, but make it quiet by default.

Consider what happens when commands fail. Here we ignore errors from cp, which means that the compress is almost certain to fail - more worryingly, we'll end up removing files that are not backed up:

echo "Copying $filename to ${arcdir}"
cp $filename ${arcdir}/${file_basename}_$(date +%Y%m%d%H%M%S)
compress ${arcdir}/$file_basename*

We should exit the script if the copy step fails. Either add || exit to the cp command, or add set -e to the script (in the latter case, we might want to proceed if compression fails - then add || true to suppress the exit.

Instead of compressing all files that begin with the basename, perhaps just compress the single file we copied. We'll need to put the target filename in a variable rather than invoking date again, so we don't get a different name.

GNU find has a -delete predicate that's more efficient than spawning rm for every match.

It's surprising that the last command includes the working directory as well as $sourcedir - make sure that users know that!


Modified code

#!/usr/bin/ksh
# Script Name: archiving.ksh
# Purpose    : To copy and  Archive the files
#-----------------------------------------------------------

# If VERBOSE is set in the environment, be chatty on stdout

set -u

die() {
    echo >& "$@"
    exit 1
}

[ $# = 2 ] || die "Usage: $0 sourcdir filename"

sourcedir=$1
arcdir=/busdata/data/archive
filename=$sourcedir/$2
archive_file=$arcdir/$(basename "$filename")_$(date +%Y%m%d%H%M%S)

if [ "${VERBOSE+x}" ]
then
    echo "$sourcedir"
    echo "$arcdir"
    echo "$2"
    echo "$filename"
    echo "$archive_file"
fi

cp ${VERBOSE+-v} "$filename" "$archive_file" || exit
compress ${VERBOSE+-v} "$archive_file"

${VERBOSE+echo "Removing files that are more than 30 days old from ${arcdir}"}
find "$arcdir" -mtime +15 -type f ${VERBOSE+-print} -delete

${VERBOSE+echo "Removing empty files from $sourcedir and $PWD"}
find "$sourcedir" . -name '858_file_*.exp' -type f -size -160k ${VERBOSE+-print} -delete
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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