On Fri, Aug 13, 2010 at 05:23, Chaitanya Yanamadala
<dr.virus.in...@gmail.com> wrote:
> Hai
> After a lot of frustration i have build this code to check which of the
> server is down and which is up.
snip
Here is a major rewrite. In order of importance, here is a list of
changes I made:
* Use whitespace properly, when in doubt use [perltidy][0] on your
code and emulate that.
* Use placeholders instead of building SQL with with values, not only
is it more efficient and general, it will also protect you from
injection attacks.
* Don't trust $@ to have a value after an eval, and use Try::Tiny
instead of block eval
* Use the strict and warnings pragmas.
* Check the return value of system calls (e.g. open, print, close, etc.).
* Use the three argument version of open, not the two argument version.
* Move the preparation of SQL out of loops, there is no need to
prepare a statement more than once.
* Use the correct equality and relational operators ( == and the like
for numbers, eq and the like for strings).
* NULL values will be undef in Perl 5, not an empty string
* All user modifiable variables should be defined near the top to make
it easy to find them, but preferably the should be moved to a config
file or passed on the commandline.
* Use lexical filehandles instead of bareword filehandles.
* Use DBI->connect()'s options to make your life easier.
* Use flow control instead of deeply nesting if statements.
* Use constants to give confusing literals meaning to the reader.
* Don't use useless temporary variables.
* Don't use concatenation where you can use interpolation.
I have left several FIXMEs in the code. They are there because I
don't have enough information to fix them or they are suggesting a
module that will simplify code and I didn't want to force more
dependencies on you (but you really should be using them).
On a separate note, your database table and column names are very bad.
Misspelling client_name, having a floating _ after status, cryptic
names like etime and and stime, and adding table to every table name
are all confusing or silly. You might also want to store the IP
address as an integer and use to inet_aton and inet_ntoa to convert it
(this has many benefits including the ability to sort by IP address).
#!/usr/bin/perl
use strict;
use warnings;
use DBI;
use XML::LibXML;
use constant ONLINE => 0;
use constant OFFLINE => 1;
#FIXME: all of this should be in a config file
#or passed on the commandline, maybe in the XML file?
my $to_email = 'dr.virus.in...@gmail.com';
my $from_email = 'cha...@server.com';
my $serverxml = "servers.xml";
my $dsn = "dbi:mysql:ping:localhost:3306";
my $threshold = 70;
print "XML NAME => $serverxml\n";
my $dbh = DBI->connect(
$dsn,
"root",
"",
{
ChopBlanks => 1, #trim selected fields
PrintError => 0, #don't print errors
RaiseError => 1, #die instead
ShowErrorStatement => 1, #and print the SQL too
}
) or die "Cannot connect to database";
my @servers;
my $parser = XML::LibXML->new();
#FIXME: consider using Try::Tiny instead
#as this construct also has problems
eval {
my $root = $parser->parse_file($serverxml)->getDocumentElement;
for my $file ($root->findnodes('/serverdetails/server')) {
push @servers, $file->findvalue('.');
}
1;
} or die "Cannot read XML\n";
my $get_sno = $dbh->prepare("
SELECT sno
FROM check_table
WHERE client_nme = ?
AND client_ip = ?
");
my $insert_check_table = $dbh->prepare("
INSERT INTO check_table (client_nme, client_ip, status_)
VALUES (?, ?, 0)
");
my $get_status = $dbh->prepare("
SELECT status_
FROM check_table
WHERE client_nme = ?
AND client_ip = ?
");
my $update_status = $dbh->prepare("
UPDATE check_table
SET status_ = ?,
stime = now()
WHERE client_nme = ?
AND client_ip = ?
");
my $insert_log = $dbh->prepare("
INSERT INTO log_table (
Check_id, client_nme, client_ip, stime, etime, status_
)
SELECT sno, client_nme, client_ip, stime, etime, status_
FROM check_table
WHERE client_nme = ?
AND client_ip = ?
");
for my $server_record (@servers) {
my ($server_name, $server_ip) = split ',', $server_record;
$get_sno->execute($server_name, $server_ip);
my $sno = $get_sno->fetchrow_array();
unless (defined $sno) {
$insert_check_table->execute($server_name, $server_ip);
}
print
"Server Name => $server_name\n",
"Server Ip => $server_ip\n";
#FIXME: this should probably be replaced with Net::Ping
my $pingstats = qx(ping -c 4 $server_ip | grep received);
my ($Received, $Lost) = (split ',', $pingstats)[1,2];
#FIXME: $received isn't used, why are we getting it?
my $received = (split ' ', $Received)[0];
my $lost = trim((split '%', $Lost)[0]);
$get_status->execute($server_name, $server_ip);
my $status = $get_status->fetchrow_array();
if ($lost >= $threshold and $status == ONLINE) {
$update_status->execute(1, $server_name, $server_ip);
sendEmail(
$to_email,
$from_email,
"SERVER DOWN.",
"Server => $server_name is Down. Check it out!!!"
);
next;
}
if ($lost < $threshold and $status == OFFLINE) {
$update_status->execute(0, $server_name, $server_ip);
$insert_log->execute($server_name, $server_ip);
sendEmail(
$to_email,
$from_email,
"SERVER ONLINE.",
"Server => $server_name is Online Now. Check it out!!!"
);
next;
}
}
#FIXME: consider replacing this with Email::Sender
sub sendEmail {
my ($to, $from, $subject, $message) = @_;
my $sendmail = '/usr/lib/sendmail';
open my $mail, "|-", $sendmail, "-oi", "-t"
or die "could not run sendmail: $!\n";
print $mail
"From: $from\n",
"To: $to\n",
"Subject: $subject\n\n",
"$message\n"
or die "could not talk to sendmail: $!\n";
close $mail
or die "could not finish talking to sendmail: $!\n";
}
sub trim {
my $string = shift;
$string =~ s/^\s+//;
$string =~ s/\s+$//;
return $string;
}
[0]: http://search.cpan.org/dist/Perl-Tidy/lib/Perl/Tidy.pm
--
Chas. Owens
wonkden.net
The most important skill a programmer can have is the ability to read.
--
To unsubscribe, e-mail: beginners-unsubscr...@perl.org
For additional commands, e-mail: beginners-h...@perl.org
http://learn.perl.org/
