Hai All
Thank you guys for your replies. I would note these points which you have
mentioned and try to use further in my coding.
Regards
Chaitanya
On Sat, Aug 14, 2010 at 5:45 AM, Chas. Owens <chas.ow...@gmail.com> wrote:
> 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.
>
