On Thursday 15 November 2007 03:57, Jeff Pang wrote:
> These two programs, monsvr and monagt, were written by Angel flower,
> who is a girl programmer, and were improved and maintained by me.
> They are a pair of socket programs, used to monitor for Linux OS's
> resource like cpu,mem,ports,swap,free disk etc.
> They have been working for more than 300 linux hosts, run long days
> without problems.
> They are easy to use, just two light perl scripts, with good features
> for monitoring for linux hosts.
> Take a look at here:
>
> http://home.arcor.de/pangj/monpub/
>
> Any criticism or suggestion are welcome.
OK, you asked for it. From the monagt program:
20 #
21 # system calls
22 #
23 my $NC = '/usr/bin/nc';
24 my $CAT = '/bin/cat';
25 my $PING = '/bin/ping';
26 my $NETSTAT = '/bin/netstat';
27 my $UPTIME = '/usr/bin/uptime';
28 my $IFCONFIG = '/sbin/ifconfig';
29 my $DF = '/bin/df';
30 my $FREE = '/usr/bin/free';
First, what is 'nc'? I have an nc program on my HD but I don't think
its the same one:
<quote>
NC(1) NC(1)
NAME
nc - Client program for NEdit text editor
</quote>
Somewhere else in the program you say it is used for port scanning.
Perhaps if you used a more widely know program like nmap?
Second, CAT? Are you trying to win the UUOC award?
And third, it may be better to use the Net::Ping module instead of the
external ping program.
70 open (PIDFILE,$pid_file) or die "[EMERG] $!\n";
79 open (HDW,">",$pid_file) or die "[EMERG] $!\n";
201 open (STDIN, "</dev/null");
202 open (STDOUT, ">/dev/null");
203 open (STDERR,">&STDOUT");
215 open (HDW,">>",$log_file);
225 open (HDW,">>",$err_log);
234 open (HDW,">>",$err_log);
Sometimes you use three arguments, sometimes two. Sometimes you check
the return value, sometimes not.
You should be consistant. You should *always* check the return value
of system functions like open(), flock(), seek(), etc.
84 # get bind addr
85 my @ifconfig = `$IFCONFIG`;
86 my ($inner_line) = grep {/inet addr\:192\.168\./ || /inet
addr\:10\./ || /inet addr\:172\./} @ifconfig;
87 my ($exter_line) = grep {/inet addr\:/ && ! /127\.0\.0\./ && !
/192\.168\./ && ! /10\./ && ! /172\./} @ifconfig;
88 my ($inner_addr) = $inner_line =~ /inet
addr\:(\d+\.\d+\.\d+\.\d+)/;
89 my ($exter_addr) = $exter_line =~ /inet
addr\:(\d+\.\d+\.\d+\.\d+)/;
You are matching the string 'inet addr:' SIX times when you need only
match it twice. Your patterns are not anchored so you could get false
negatives, for example: the address 64.37.172.85 is a valid internet
address yet the pattern /172\./ would exclude it (/10\./ and
/192\.168\./ have the same problem.) AFAIK all 127/8 addresses are
local, not just 127.0.0/24. According to RFC 1918:
<quote>
3. Private Address Space
The Internet Assigned Numbers Authority (IANA) has reserved the
following three blocks of the IP address space for private internets:
10.0.0.0 - 10.255.255.255 (10/8 prefix)
172.16.0.0 - 172.31.255.255 (172.16/12 prefix)
192.168.0.0 - 192.168.255.255 (192.168/16 prefix)
We will refer to the first block as "24-bit block", the second as
"20-bit block", and to the third as "16-bit" block. Note that (in
pre-CIDR notation) the first block is nothing but a single class A
network number, while the second block is a set of 16 contiguous
class B network numbers, and third block is a set of 256 contiguous
class C network numbers.
</quote>
so your /172\./ pattern (even if it were anchored correctly) would
exclude some valid internet addresses.
309 sub get_free_mem
310 {
311 my $arg = shift;
312 my ($sign,$mem_free) = $arg =~ /([+-])([\d\.]+)/;
313
314 my $warn;
315 unless (defined $sign && defined $mem_free) {
316 $warn = "uncorrect argument in config file";
317 return $warn;
318 }
319
320 my @meminfo = `$CAT /proc/meminfo`;
370 sub check_swap
371 {
372 my $arg = shift;
373 my ($sign,$swap) = $arg =~ /([+-])(\d+)/;
374
375 my $warn;
376 unless (defined $sign && defined $swap) {
377 $warn = "uncorrect argument in config file";
378 return $warn;
379 }
380
381 my @meminfo = `$FREE`;
You use /proc/meminfo in one place and free in the other but all of the
same information is available from both so why two different methods?
239 sub kill_children
240 {
241 kill TERM => keys %status;
242 sleep while %status;
243 }
You are not modifying %status in the sub so will this ever return?
John
--
use Perl;
program
fulfillment
--
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
http://learn.perl.org/
