when you say the current iteration of the loop, do you mean any if/else statements inbetween the do and done commands?
Yes. "loop" means the for-loop and whatever is between "do" and "done" constitutes the loops "body" - the part which is executed once for each iteration.
You would actually be able to see what this loop body is, given you'd stop on ignoring the numerous suggestions to indent your code! This is not about some idle "make it nicer" suggestion. In fact this is a technique developed in decades by generations of programmers writing code proven to work in giving more oversight about the structure of ones program. And before you ask: of course it possible not to do it and still end up with working code. It is also possible to turn left with your car without blinking. But like turning without blinking will increase the chance of having an accident this increases the chance of you losing oversight.
Quote:
Originally Posted by robertkwild
i will make it neater near when im nearly done finalizing it but its looking good
No! you should start writing your code in a neat form, not write it in bad/unreadable form first and make it neat in the end. You need every help to organize your code while you are writing it, not after writing.
Btw., as Don Cragun has mentioned he uses a different indenting style than me. Which one (his, mine or your genuine one) doesn't matter - as long as you use one! I can read Don Craguns code as well as my own (with a possible minimal understanding speed penalty so small i do not even notice) and it is probably the same the other way round. But your code is really hard to read simply because the eye has no "holding point". It is like removing all paragraphs from a text: yes, it is still readable, but perhaps more hard to read.
Finally there is another point: you use a lot of repeating text in your script:
Code:
[...]
mail -s "${fSaveDir}" robertw@molinare.co.uk <<< "creating of tar
[...]
mail -s "${fSaveDir}" robertw@molinare.co.uk <<< "rsync of "${fSaveDir}" failed due to error"
[...]
mail -s "${fSaveDir}" robertw@molinare.co.uk <<< "removing of tar "${fSaveDir}" failed due to error"
[...]
Suppose you want to change the email-address to something else. You would have to go through your code and change every instance of it, right?
For such repeating patterns you might consider using a variable. Look at the following example script:
Code:
#! /bin/bash
chMailAddress="robertw@molinare.co.uk"
mail -s "subject 1" "$chMailAddress" <<< "first message"
mail -s "subject 2" "$chMailAddress" <<< "second message"
mail -s "subject 3" "$chMailAddress" <<< "third message"
exit 0
If you would want to change the recipients mail address you'd simply change the value of "chMailAddress" at the top - less work and you will be sure to have it changed everywhere and not forgotten one line, no?
So, as a rule of thumb, you look for such repeating patterns (in this case "text constants") and strive to put them into variables ("constants" are simply variables which never change their value).
#!/bin/bash
cd /to_be_archived/
for DIR in * ;
do
SaveDir="${DIR##*/}"
MailAddress="robertw@molinare.co.uk"
tar -cf "$SaveDir".tar "$SaveDir"
if [ $? -ne 0 ]
then
mail -s "failed "$SaveDir" " $MailAddress <<< "creating of tar "$SaveDir" failed due to error, removing "$SaveDir".tar"
rm -f "$SaveDir".tar
continue
fi
rsync -a "$SaveDir".tar /archived_projects/
if [ $? -ne 0 ]
then
mail -s "failed "$SaveDir" " $MailAddress <<< "rsync of "$SaveDir" failed due to error"
continue
fi
rm -f "$SaveDir".tar
if [ $? -ne 0 ]
then
mail -s "failed "$SaveDir" " $MailAddress <<< "removing of tar "$SaveDir" failed due to error"
continue
fi
rm -rf "$SaveDir"
if [ $? -ne 0 ]
then
mail -s "failed "$SaveDir" " $MailAddress <<< "removing of "$SaveDir" failed due to error"
continue
else
mail -s "success "$SaveDir" " $MailAddress <<< "successfully completed archiving "$SaveDir" "
fi
done
Last edited by robertkwild; 04-18-2016 at 01:50 PM..
Please indent your code to show its structure! Adding blank lines (instead of indenting) still hides the fact that all of your if statements, tar commands, rsync commands, rm commands, and variable assignments are inside a loop and that your mailx commands and continue statements are conditional in your if then or else clauses. And, it hides the fact that you set MailAddress every time you go through your loop when it just needs to be set once before your loop starts. And separating commands like tar into a different "paragraph" than the code that tests its return value gives a visual cue that it is OK to add other code between them. (But, if you do add any code there the following "paragraph" testing the return code of your tar. rsync, and rm commands will NOT work as desired and there will be no indication that a logic error in your code has been introduced!)
I still question why you want one e-mail per directory processed instead of one e-mail containing the status of all of the directories you are processing, but that it is up to. (Note also that the way I suggested doing it, any status or diagnostic messages from tar, rsync, and rm would have appeared in you e-mail as a permanent record of what happened. The way you are doing it, any output from those commands will appear on your terminal, but will not be part of your permanent record in the archival status stored in you mailbox.)
I repeat that, the way your loop is written, the DIR and SaveDir variables will ALWAYS have the same value. Why use two variables when they can never hold different values? (It forces me to look at your entire script to try to find where you set one of them to have a different value.)
And, as stated before, even though you name your variables DIR and SaveDir, your code creates regular files that are sometimes left lying around that later runs of your script should ignore (or write a diagnostic message telling you that they should be removed). (For example, if the tar succeeds and the rsync fails, a .tar file will be left around. And, you are already noting that rm can fail leaving a .tar lying around. With your code, in these cases, the next time you run your code you'll create a directory.tar.tar file and try to rsync it to your archive area.)
Note that variables are expanded inside double-quoted strings. The command:
Code:
mail -s "failed "$SaveDir" " $MailAddress <<< "... "$SaveDir" failed due to error"
should be written as:
Code:
mail -s "failed $SaveDir" $MailAddress <<< "... $SaveDir failed due to error"
to protect yourself from serious side effects that will jump up and bite you if one of your directory names ends up containing a whitespace character at some point in the future.
Please indent your code to show its structure! Adding blank lines (instead of indenting) still hides the fact that[...]
I absolutely, wholeheartedly, fully, completely concur! *)
But probably we can't drive home the fact of just how important indentation is because the script is still in a stage where the structure is very very basic. So, in a last-ditch effort, before i am out of here, here is an excerpt of a real production-level script i use, first without indentation:
Code:
while [ -n "$1" ] ; do
chAttr="${1%%=*}"
chValue="${1#*=}"
shift
case "$chAttr" in
"name") # user name
chRemUser="$chValue";;
"hostname") # host name
(( iRemHostPtr += 1 )) # add new element to array
typeset achRemHost[$iRemHostPtr]="$chValue";;
*) (( iAttr = 1 ))
(( lFoundAttr = 0 ))
while [ $iAttr -le ${#achAttr[*]} -a $lFoundAttr -eq 0 ] ; do
if [ "${achAttr[$iAttr]}" == "$chAttr" ] ; then
# protect values with blanks
if [ "$chValue" == "${chValue% *}" ] ; then
achValue[$iAttr]="$chValue"
else
achValue[$iAttr]="'$chValue'"
fi
(( lFoundAttr = 1 ))
fi
(( iAttr += 1 ))
done
if (( lFoundAttr == 0 )) ; then
if (( lStrictChecking == 1 )) ; then
f_CmdError "Unknown Attribute $chAttr ($chValue) found. Aborting:"
f_die 1
else
f_CmdWarning "Ignoring unknown Attribute $chAttr ($chValue)"
fi
fi
;;
esac
done
You might want to compare that drivel to the original:
Code:
while [ -n "$1" ] ; do
chAttr="${1%%=*}"
chValue="${1#*=}"
shift
case "$chAttr" in
"name") # user name
chRemUser="$chValue"
;;
"hostname") # host name
(( iRemHostPtr += 1 )) # add new element to array
typeset achRemHost[$iRemHostPtr]="$chValue"
;;
*)
(( iAttr = 1 ))
(( lFoundAttr = 0 ))
while [ $iAttr -le ${#achAttr[*]} -a $lFoundAttr -eq 0 ] ; do
if [ "${achAttr[$iAttr]}" == "$chAttr" ] ; then
# protect values with blanks
if [ "$chValue" == "${chValue% *}" ] ; then
achValue[$iAttr]="$chValue"
else
achValue[$iAttr]="'$chValue'"
fi
(( lFoundAttr = 1 ))
fi
(( iAttr += 1 ))
done
if (( lFoundAttr == 0 )) ; then
if (( lStrictChecking == 1 )) ; then
f_CmdError "Unknown Attribute $chAttr ($chValue) found. Aborting:"
f_die 1
else
f_CmdWarning "Ignoring unknown Attribute $chAttr ($chValue)"
fi
fi
;;
esac
done
And if this won't let you see the light perhaps nothing will.
I hope this helps.
bakunin
____________________
*) And that i stopped agreeing here was only because i ran out of pleonasms. ;-)
because i want to be notified straight after the command the return code, if its 0 continue if its not 0 email me straight away, i dont want to print the values and right at the end get an email with any command its failed i want to know straight away
do you think this isnt good practice then?
and sorry what do you mean by dir and savedir are the same, as i cant see any dir name
Code:
#!/bin/bash
cd /to_be_archived/
for DIR in * ;
do
SaveDir="${DIR##*/}"
MailAddress="robertw@molinare.co.uk"
tar -cf "$SaveDir".tar "$SaveDir"
if [ $? -ne 0 ]
then
mail -s "failed $SaveDir" $MailAddress <<< "creating of tar $SaveDir failed due to error, removing $SaveDir.tar"
rm -f "$SaveDir".tar
continue
fi
rsync -a "$SaveDir".tar /archived_projects/
if [ $? -ne 0 ]
then
mail -s "failed $SaveDir" $MailAddress <<< "rsync of $SaveDir failed due to error, removing $SaveDir.tar"
rm -f "$SaveDir".tar
continue
fi
rm -f "$SaveDir".tar
if [ $? -ne 0 ]
then
mail -s "failed $SaveDir" $MailAddress <<< "removing of tar $SaveDir failed due to error"
continue
fi
rm -rf "$SaveDir"
if [ $? -ne 0 ]
then
mail -s "failed $SaveDir" $MailAddress <<< "removing of $SaveDir failed due to error"
continue
else
mail -s "success $SaveDir" $MailAddress <<< "successfully completed archiving $SaveDir"
fi
done
---------- Post updated at 10:07 AM ---------- Previous update was at 08:04 AM ----------
yeah your right, i didnt need that variable, so i deleted it
Code:
SaveDir="${DIR##*/}"
and changed all the
Code:
$SaveDir
to
Code:
$DIR
also the last if/else statement under the else im going to print/echo the values to a file so i have a hard copy aswell as email
i will let you guys know
thank you for your on going help, much appreciated
command
if [ $? -eq 0 ] ; then
do_something_success
else
do_something_failed
fi
as you do you can simply write:
Code:
if command ; then
do_something_success
else
do_something_failed
fi
which does absolutely the same, just without the risk of putting some other command in between and changing "$?" from the return of one command to the return code of another,as Don Cragun mentioned earlier. Personally i find that style to be easier to follow.
because i want to be notified straight after the command the return code, if its 0 continue if its not 0 email me straight away, i dont want to print the values and right at the end get an email with any command its failed i want to know straight away
do you think this isn't good practice then?
... ... ...
also the last if/else statement under the else im going to print/echo the values to a file so i have a hard copy aswell as email
... ... ...
cheers
rob
If you want to know right away that an error occurred while running your script, watch your script run. Using e-mail to be notified of time-critical events is strange. (There is no guarantee that an e-mail message will be delivered in any particular bounded time.) And the messages you're sending indicate that something went wrong, but you don't include the exit code and you don't include any diagnostic messages produced by any failing commands; so you know something went wrong, but you have no idea what the failing command told you when it failed. So, if anything goes wrong, you are throwing away most of the information that would help you figure out what needs to be fixed unless you are watching the script run (in which case, the e-mail gives you late notice of what you already know). And, assuming that most of your archiving activities complete successfully, you also get an urgent e-mail saying that each archival succeeded. After you've run this a few times, will you really notice an e-mail saying a project archival failed when you're used to seeing an e-mail saying a project archival succeeded?
With all of the things a sys admin has to do every day, is failing to archive a completed project high on the list of things that have to be addressed immediately?
The code I suggested gave you a single e-mail with all of the diagnostics produced by any failing commands as well as your failure and success notes. Your code gives you one e-mail containing a single line failure or success note for each attempted archival (throwing away any diagnostics produced by failing commands).
Obviously, you can do what you want to do here. If I were administering attempts to archive projects, I know which I would prefer.
- Don
PS You might also consider consolidating your remove commands:
Code:
rm -f "$SaveDir".tar
and
rm -rf "$SaveDir"
into the single command:
Code:
rm -rf "$DIR" "$DIR.tar"
If you successfully tar a project's files and successfully rsync that archive to your archive server, is there any reason not to try to remove the project's files even if the attempt to remove the tar archive failed?
HI Guys,
I have some 8 files with different name and extensions. I need to check if they are present in a specific folder or not and also want that script to show me which all are not present. I can write if condition for each file but from a developer perspective , i feel that is not a good... (3 Replies)
1. The problem statement, all variables and given/known data:
The task is to create a script that would reproduce the output of 'du' command, but in a different way: what 'du' does is:
<size> <folder name>and what is needed is
<folder name> <size>We need to show only 10 folders which are the... (3 Replies)
Good day, everyone!
I'm very new to bash scripting. Our teacher gave us a task to create a script that basically does the same job the 'du' command does, with the difference that 'du' command gives an output in the form of
<size> <folder name>and what we need is
<folder name> <size>As for... (1 Reply)
Hi All,
I have to requirement to write a shell script to move file from one folder (A) to another five folder (B,C,D,E,F) and destination folder should be blank. In not blank just skip.
This script will run as a scheduler every 2 minutes. It will check number of files in folder A and move 1 to... (9 Replies)
I would like to confirm my file.tar is been tar-ed correctly before I remove them. But I have very limited disc space to untar it.
Can I just do the listing instead of actual extract it? Can I say confirm folder integrity if the listing is sucessful without problem?
tar tvf file1.tar
... (1 Reply)
Hi all,
I have a tar file and inside that tar file is a folder with additional tar.gz files. What I want to do is look inside the first tar file and then find the second tar file I'm looking for, look inside that tar.gz file to find a certain directory. I'm encountering issues by trying to... (1 Reply)
I have 1000's of directories which is named as numbers. Each directory contains multiple files. Each of these directories have a file named "att". I need to rename all the att files by adding the directory name followed by "_" then att for each of the directories.
Directories
120
att... (2 Replies)
I have in directory /media/AUDIO/WAVE many .mp3 files with names like:
my filename_01of02.mp3
my filename_02of02.mp3
Your File_01of06.mp3
Your File_02of06.mp3
etc....
In the same directory, /media/AUDIO/WAVE, I have many folders with names like
9780743579490
9780743579491
etc..
Inside... (7 Replies)
I have written a shell script to perform backups using tar, rsync and optionally utilise lvm snapshots. The script is not finished but is in a working state and comments/descriptions are poor.
I would greatly appreciate any criticism and suggestions of the script to help improve my own learning... (0 Replies)
Hi.
I'm trying to copy multiple folders from the remote machine to the local machine. I wrote a batch file to run an ftp window.
The problem I am having is that the only command to copy files is mget *, and this copies only files, not folders.
For example, ftp ts555
cd ts555/test
' test... (5 Replies)
