Originally posted by: Wouter Liefting
As far as the errors in your script are concerned, here's a list of things that I stumbled upon.
#!/bin/ksh
^^^^ That's actually very good practice. Particularly since you're using the "print" statement later on, which is exclusive to ksh.
touch myips.txt myports.txt result.txt
rm myips.txt myports.txt result.txt
^^^^ Why touch the files and then delete them? If you want to make sure they're writable, you need some error handling as well. I always declare a function die():
die()
{
print "$@"
exit 1
}
Error handling then becomes:
touch myips.txt || die ("Cannot create myips.txt")
Furthermore, it's good practice to put the filenames in variables, and use these variables throughout the script. Much easier if you need to change the filename or directory later on. And it's good practice to create them in /tmp instead of the current working directory, give them a name that's unique even if two instances of your script run in parallel (use $$ as part of the filename, or use mktemp), and delete them afterwards. So your code becomes:
myips=/tmp/myips.txt.$$
[ -f "$myips" ] && die "File $myips already exists"
touch "$myips" || die "Cannot create $myips"
...
echo "bla bla bla" >> "$myips"
...
rm -f "$myips" || die "Cannot delete $myips"
read ip1?"insert dest ip: "
read p1?"insert first port of the range (or the unique one): "
read pn?"insert last port of the range (or the unique one): "
^^^ Why read from the command line? Why not use the script arguments $1, $2, $3 and so forth. Do need to add a proper usage statement though, in case the parameters are not what you expected.
echo $ip1 >> myips.txt
echo $p1 $pn | awk '{ for (i=$1;i<=$2;i++) { print i} }'>> myports.txt
for ip in $('cat' myips.txt)
do
for port in $('cat' myports.txt)
^^^^ First, it doesn't make sense to enclose cat in single quotes. They don't add anything here. However, it is good practice to either set the $PATH variable yourself to a sensible value, or hard-code the path with the command (/bin/cat). Now you're completely depending on the user setting an appropriate $PATH variable.
Furthermore, the use of these temporary files is not needed at all. You could have done this as well:
for ip in $ip1
do
for port in $( echo "$p1" "$pn" | awk '{ for (i=$1;i<=$2;i++) { print i} }' )
do
....
done
done
And I personally prefer to use the tool "seq" for this instead of awk, but seq might not be available on AIX (can't check right now). Still, you could have avoided the convoluted use of awk with a while loop:
for ip in $ip1
do
curport="$p1"
while [ "$curport" -lt "$pn" ]
do
....
curport=$(( "$curport" + 1 ))
done
done
(Also note that the $ip1 is deliberately not enclosed in double quotes. This is because this shell variable might contain multiple IP addresses, separated by spaces, and I want to interpret this as multiple IP addresses, not as a single value. It's one of the very few exceptions to the double-quotes rule.)
do
echo ""
echo ""
echo "telnet $ip $port" >> result.txt
^^^ You're displaying good practice here. Anytime you refer to a shell variable, you do so within double quotes. That saves the structure, even if the shell variable contains spaces. Keep it up.
sleep 1 | telnet $ip $port >> result.txt 2>&1 &
^^^ Well, never mind. You forgot the double quotes here already... telnet "$ip" "$port" would've been better although it doesn't matter in this particular case. But if you start doing the same thing with filenames that possibly contain spaces (the dreaded My Documents for instance) then you're screwed.
echo "Telnet in progress..saving results to "result.txt""
^^^ Lucky for you this did not generate a syntax error. But the quotes around result.txt are not printed, and in other situations this might actually have caused errors. Next time, try this instead:
echo "Telnet in progress... Saving results to \"result.txt\"."
PID=$!
^^^^If you do anything with variables $!, $? and such, it's good practice to recall and store them straightaway, before running any other command which could possibly alter their values. So swap the PID=$! and echo lines.
Furthermore, user shell variables are normally written in lowercase, to avoid confusion/conflict with system shell variables which are always in uppercase.
sleep 2
if [ $(ps -efa | grep $PID |grep -v grep|grep -v defunct| wc -l) -eq 1 ]; then
TMOUT_CONN=$(ps -efa | grep $PID |grep -v grep|grep -v defunct| awk '{print $9,$10}')
^^^^ Very, very inefficient. You call ps twice and grep six times. All this can be done in one line:
TMOUT_CONN=$(ps -efa | awk -v pid=$PID '$2 == pid {print $9 $10 }')
The "-v pid=$PID" assigns the awk variable pid the value of shell variable PID. From then one you can use the awk variable pid inside the single quotes. Otherwise you'd have to use the shell variable $PID inside the awk code block, but that's quoted with single quotes so shell variable expansion doesn't work. You can circumvent that, but it makes for very messy code. This is much cleaner.
The '$2 == pid {print $9 $10}' block means: evaluate each line. If the second column equals the pid variable, then print column 9 and 10. As the second column from ps -efa contains the pid, you can be sure to match the correct PID. You don't need to filter out the accidental inclusion of the "grep $PID" command for instance.
kill -9 $PID
^^^ Normally not a good idea to kill something with a -9 straight away. Better to do a -15 first, then wait a few seconds, then a -9. But I can see the logic here.
echo "PID $PID still UP after 2 seconds, connection to $TMOUT_CONN is in Trying state.\nSubprocess Killed"
fi
done
done
The logic error here is that the script will only detect a filtered port (timeout). It doesn't do anything with the exit value of telnet, which would possibly give you enough information to distinguish between an open and a closed port. To retrieve the return code of telnet, you'd have to insert a 'wait' statement somewhere along the line.
But having said that, I don't think telnet distinguishes between the three situation (port was open but the connection was closed because stdin closed, port was filtered leading to a timeout and telnet was killed with a -9 signal, port was closed and telnet exited by itself). In fact, with a kill -9 telnet would not be able to leave a sensible exit code in any case. So I would not build my script around telnet in any case.