5
\$\begingroup\$

I wrote up script for backup of file system's log files, Please suggest me if any modification can be done on it for better working. (script)

#!/bin/sh
##### SCRIPT CREATED AND MAINTAINED BY #<Keshav Piplani> ######
#####FOR ANY QUERIES PLEASE CONTACT <[email protected]> ######

#Directory under which files backup needs to be taken
DIR="/data/jboss6/jboss-6.1.0.Final/server/default/log"
#backup location
backup="/data/jboss6/sonata_log_backup"
DAY=`date +%d`              
MONTH=`date +%m`           
YEAR=`date +%Y`           
MIN=`date +%M`
SEC=`date +%S`
date=`date +%F`

#Directory under which files backup needs to be taken

home="/data/jboss6/jboss-6.1.0.Final/server/default/log"
#script output folder
script="/data/jboss6/sonata_log_backup/dailyscript_output"
#1day old files
days=1
mkdir -p "$backup/dailyscript_output"
mkdir -p "$backup/dailyscript_output"/"$date"
mkdir -p  "$backup"/log/
mkdir -p  "$backup"/log/"$date"

mkdir -p "$backup/dailyscript_output" 2>>/$script/$date/error_log
mkdir -p "$backup/dailyscript_output"/"$date" 2>>/$script/$date/error_log
mkdir -p  "$backup"/log/ 2>>/$script/$date/error_log
mkdir -p  "$backup"/log/"$date" 2>>/$script/$date/error_log

files=($(find $DIR -maxdepth 1 -type f  -mtime +"$days" | grep -v gz | grep -i log )) 2>>/$script/find_error_log
echo "=================================================================="
echo "Zipping Files "
echo "Start  of script `date` "

echo "------------------------------------------------$date------------------------------------------------" >>"$script/$date"/In_use@"$DAY.$MONTH.$YEAR"notzipped.txt
echo "------------------------------------------------$date------------------------------------------------" >>"$script/$date"/zip_details_"$YEAR"_"$MONTH"_"$DAY".txt
echo "------------------------------------------------$date------------------------------------------------" >>"$script/$date"/onedayoldfiles_"$YEAR"_"$MONTH"_"$DAY".txt

for files in ${files[*]}
do


# Check whether files are in use (might still be being copied into directory)
/sbin/fuser "$files" > /dev/null 2>&1
fuser_output=`echo $?`


ls -l $files>>"$script/$date"/onedayoldfiles_"$YEAR"_"$MONTH"_"$DAY".txt
if [[ "$fuser_output" != "0" ]]; then


   #  echo $files
#zip files details in zip_details_date.txt
#ls -l $files>>"$script/$date"/zip_details_"$YEAR"_"$MONTH"_"$DAY".txt

 gzip "$files" 
 mv -i "$files".gz "$files"zipped@"$DAY.$MONTH.$YEAR".log.gz
ls -l "$files"zipped@"$DAY.$MONTH.$YEAR".log.gz | sed 's/\/log/\/log\/old_log_backup\/'$date'/g' >>"$script/$date"/zip_details_"$YEAR"_"$MONTH"_"$DAY".txt
 mv -i  "$files"zipped@"$DAY.$MONTH.$YEAR".log.gz "$home"/old_log_backup/"$date"/

else
         echo "$files is in use as on $date" >>"$script/$date"/In_use@"$DAY.$MONTH.$YEAR"notzipped.txt 
    echo " " >>$script/email"$date".txt

fi


done

all=`cat "$script/$date"/1dayoldfiles_"$YEAR"_"$MONTH"_"$DAY".txt | grep -v "$date" | wc -l `

Total=`cat "$script/$date"/zip_details_"$YEAR"_"$MONTH"_"$DAY".txt `
Totalno=`cat "$script/$date"/zip_details_"$YEAR"_"$MONTH"_"$DAY".txt |grep -v "$date-" | wc -l `
inuseno=`cat "$script/$date"/In_use@"$DAY.$MONTH.$YEAR"notzipped.txt | grep -v "$date-" | wc -l `
inuse=`cat "$script/$date"/In_use@"$DAY.$MONTH.$YEAR"notzipped.txt `
#echo $Total

echo "Start of Email " >>$script/email"$date".txt

echo " " >>$script/email"$date".txt
#echo " " >>$script/email.txt
echo "Zip file location:  $home/old_log_backup  at Box `hostname` " >>$script/email"$date".txt
echo " " >>$script/email"$date".txt
echo "Report Output stored in : $script  at Box `hostname`" >>$script/email"$date".txt
echo " " >>$script/email"$date".txt
echo  " "$all" files are  1day old,  See Below --- :">>$script/email"$date".txt

echo " `cat "$script/$date"/1dayoldfiles_"$YEAR"_"$MONTH"_"$DAY".txt ` " >>$script/email"$date".txt
echo " " >>$script/email"$date".txt

echo  "Total $Totalno Files got Zipped to $home/old_log_backup/$date , See Below ---" >>$script/email"$date".txt
echo   "$Total" >>$script/email"$date".txt
echo " " >>$script/email"$date".txt

echo  " $inuseno Files are in use are Below " >>$script/email"$date".txt
echo  " $inuse " >>$script/email"$date".txt
echo " " >>$script/email"$date".txt

echo "End of Email " >>$script/email"$date".txt
echo "SCRIPT CREATED BY  <[email protected]> " >>$script/email"$date".txt


cat $script/email"$date".txt | mailx -s "FILE Got zipped to 'old_log_backup' directory  on date $date for server `hostname` " "[email protected]"
>$script/email.txt

echo "End of script `date`"
echo "=================================================================="
\$\endgroup\$
6
  • \$\begingroup\$ (Welcome to SO!) Well, the guide-line is to Be sure to embed the code you want reviewed in the question itself - not being able to do so due to code size is a hint in itself: it is too big. So, why are you not able to paste whole script, while you can publish it? Can we do something to change that? (On inspection of the contents linked: make me think you an owner or maintainer of the code.) \$\endgroup\$
    – greybeard
    Jan 6, 2018 at 10:26
  • \$\begingroup\$ yes i am owner of that script. pasting here giving me error like too longs that's why embedded a URL here \$\endgroup\$
    – Peter
    Jan 6, 2018 at 11:40
  • \$\begingroup\$ (Originally posted on serverfault.) \$\endgroup\$
    – greybeard
    Jan 6, 2018 at 11:43
  • 1
    \$\begingroup\$ (See second line of script for a motive to ask affirmation of custodianship.) \$\endgroup\$
    – greybeard
    Jan 6, 2018 at 11:50
  • 1
    \$\begingroup\$ @greybeard This is Code Review (CR), not Stack Overflow (SO). Please don't confuse the new users. \$\endgroup\$
    – Mast
    Jan 20, 2018 at 13:32

1 Answer 1

2
\$\begingroup\$

Nice to see you here at Code Review.

There are lots of things to talk about, so let's get started.


But before we actually get to the code we'll deal with comments and formating.

The first one you used is called "shebang". I like that you used #!/bin/sh but not bash, zsh or whatever interactive shell is there on your system. Try stick to sh, most of the time it is all you need. Besides /bin/sh is often a symlink to dash shell, which is pretty fast and POSIX compilant.

It is matter of personal style, but try not to use all caps for regular comments. This is really annoying. To get reader attention add one more newline before and after your comment.

 

See, that works just fine.

 

Try to place at least one space after #. This way your comments will be a lot more readable. Consider this:

#!/bin/sh

# Created by <Keshav Piplani>
# You can contact me via <[email protected]>


# This script will move some system log files around.

...

Last but not least: indent your code.

 

Aha, I see that you don't follow any particular naming convention. Some of your variables are UPPERCASE= others are lowercase=.

Also try to use meaningful variable names. Don't make them short, don't drop random characters from them.

And most important: make your choice and be consistent.

Pick one, that you like:

SOURCE_DIR="/data/jboss6/jboss-6.1.0.Final/server/default/log"
TARGET_DIR="/data/jboss6/sonata_log_backup"

or

source_dir="/data/jboss6/jboss-6.1.0.Final/server/default/log"
target_dir="/data/jboss6/sonata_log_backup"

I'll use the latter.


See this two variable declarations:

home="/data/jboss6/jboss-6.1.0.Final/server/default/log"
script="/data/jboss6/sonata_log_backup/dailyscript_output"

home variable is exact copy of source_dir defined several lines above. I guess one of them is unnecessary. script may be set to "$target_dir/dailyscript_output", no need to use same path twice.

In short: don't repeat yourself.


mkdir -p "$backup/dailyscript_output"
mkdir -p "$backup/dailyscript_output"/"$date"
mkdir -p  "$backup"/log/
mkdir -p  "$backup"/log/"$date"

mkdir -p "$backup/dailyscript_output" 2>>/$script/$date/error_log
mkdir -p "$backup/dailyscript_output"/"$date" 2>>/$script/$date/error_log
mkdir -p  "$backup"/log/ 2>>/$script/$date/error_log
mkdir -p  "$backup"/log/"$date" 2>>/$script/$date/error_log

mkdir -p or mkdir --parents will create all missing path components. So the upper may be replaced with this:

mkdir -p \
  "$target_dir/dailyscript_output/$date" \
  "$target_dir/log/$date"

It is very tedious to redirect each command to log file. You may redirect your script stderr to some log. Also you may wrap all your script body with braces and redirect them instead:

#!/bin/sh

# comments...

{

    ...

} 2>"$logfile"

 

Try to follow suggested points and rewrite your code. After that you may create a follow up question with your new code to see your progress.

\$\endgroup\$

Your Answer

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

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