Hello,
I have several locations with a hardware VPN device and
I've set them up to log to a central logging server.
Some of the managers of these facilities would like to
have a report showing who's using the VPN and when.
I've written a script that does that. It works, but as
I'm pretty new at Perl (this is only my second real
life application) I'm sure it could be improved
(significantly).
Would some kind soul like to take a look at this
script, make suggestions or otherwise completely
destroy my ego? :)
Thanks in advance to anyone who takes this up
The script can be found at:
And some fake (sanitized) data to run it against (you'll
have to make changes to the script accordingly):
(60k text file, some
gunk has been pre-filtered)
HINT: you can change $date in the script to get
more hits.
Thanks again,
(K)

Friday 06 May 2005 22:41, Jason Balicki wrote:
Hello,

I have several locations with a hardware VPN device and
I've set them up to log to a central logging server.

Some of the managers of these facilities would like to
have a report showing who's using the VPN and when.

I've written a script that does that. It works, but as
I'm pretty new at Perl (this is only my second real
life application) I'm sure it could be improved
(significantly).

Would some kind soul like to take a look at this
script, make suggestions or otherwise completely
destroy my ego? :)

Thanks in advance to anyone who takes this up

The script can be found at:

Two quickies to save you time:

1. Use File::Temp %3A%3Atemp&mode=all
2. Use HERE documents to save loads of print statements eg

To begin a here document, follow print by the "<<" operator and a label

print << EF;
my $stuff here
EF

My tuppence.

Gavin.

And some fake (sanitized) data to run it against (you'll
have to make changes to the script accordingly):

(60k text file, some
gunk has been pre-filtered)

HINT: you can change $date in the script to get
more hits.

Thanks again,

(K)

Jason Balicki wrote:
Hello,

Hello,

I have several locations with a hardware VPN device and
I've set them up to log to a central logging server.

Some of the managers of these facilities would like to
have a report showing who's using the VPN and when.

I've written a script that does that. It works, but as
I'm pretty new at Perl (this is only my second real
life application) I'm sure it could be improved
(significantly).

Would some kind soul like to take a look at this
script, make suggestions or otherwise completely
destroy my ego? :)

, you asked for it.

Thanks in advance to anyone who takes this up

The script can be found at:

And some fake (sanitized) data to run it against (you'll
have to make changes to the script accordingly):

(60k text file, some
gunk has been pre-filtered)

Thank you very much for providing a working program and data to test it with.

#!/usr/bin/perl -w
#
# FIXME: change the tempfile to something sane (randomly generated)
# FIXME: delete the tempfile when we're done

If you really need a temp file you should use the File::Temp module.

perldoc File::Temp

#
use warnings;

You don't need to use both the '-w' switch and the 'warnings' pragma, remove
the '-w' switch.

use strict;
my $inputfile = '/var/log/messages';
my $tmp_file = '/tmp/vpntmp';
my $month = `date +%b`;
my $day = `date +%-d`;
#my $day =140;
my $year = `date +%Y`;

There is no need to call an external program to get the date as perl provides
functions for this:

my ( $month, $day, $year ) = localtime =~ /(\w{3})\s+(\d+)\s+\S+\s+(\d{4})/;

my @pppd_pids;
my $pppd_pid_index=0;
chop ($month, $day, $year);

You should use chomp() instead of chop() (which you rarely need) and you won't
need either if you use the code above.

open(FILE,$inputfile);

You should *ALWAYS* verify that the file opened correctly.

# generate temp file
open (TEMPFILE,">$tmp_file");

You don't really need a temp file as one days data should fit easily into
memory.

print "VPN usage report for $day $month $year\n\n";
# populate temp file and get pids
while (<FILE>) {
#because of the split()s I need to convert all
#multiple spaces to single spaces.
tr/ //s;

If you used split() correctly then you wouldn't need to do this.

# populate temp file, if it's a pppd line from the 550
if ((/$month $day/ && /$year/ && /SG550/ && /pppd/) || (/$month $day/ && /$year/ && /SG550/ && /pptpd/)) {

You can simplify that a bit:

if ( /$month $day/ && /$year/ && /SG550/ && /ppt?pd/ ) {

print TEMPFILE $_;
}
# if it's a connection note the pid for later processing
#if (/$month $day/ && /$year/ && /pppd/ && /peer authentication succeeded/){
if (/$month $day/ && /$year/ && /pppd/ && /2.3.8 started by/){

You are including the pppd version number in the regular expression but that
won't work if you upgrade pppd. Also, the period is special in regular
expressions, it matches any character.

#my $converted_string = $_;
#$converted_string =~ tr/ //s;
my @fields = split(/ /,$_);

You are using split() to split on a single space character. If you used the
default split with no arguments it will work better:

my @fields = split;

my $pppd_pid = get_pid($fields[5]);
$pppd_pids[$pppd_pid_index]=$pppd_pid;
$pppd_pid_index++;

Instead of keeping track of the array index just use push():

push @pppd_pids, $pppd_pid;

}
}
close ( TEMPFILE );
close ( FILE );

Instead of critiquing the rest of your code I'll show you how I would code it:

#!/usr/bin/perl
use warnings;
use strict;

my $inputfile = '/var/log/messages';
my ( $month, $day, $year ) = localtime =~ /(\w{3})\s+(\d+)\s+\S+\s+(\d+)/;

open FILE, '<', $inputfile or die "Cannot open $inputfile: $!";

print "VPN usage report for $day $month $year\n\n";

my ( $remote, %pids );
while ( <FILE) {
next unless /^$month +$day +([\d:]+).+?SG550 +ppt?pd\[(\d+)].+?\($year/o;
my ( $time, $pid ) = ( $1, $2 );

if ( /CTRL: Client ([\d.]+) control connection started/ ) {
$remote = $1;
}
elsif ( /pppd [\d.]+ started by/ ) {
# Instead of guessing the PID difference between pppd and pptpd
# this assumes that the pppd 'started by' line comes right after
# the pptpd 'connection started' line.

@{ $pids{ $pid } }{ qw/start remote/ } = ( $time, $remote );
}
elsif ( /peer authentication succeeded for (.+)/ ) {
$pids{ $pid }{ username } = $1;
}
elsif ( /remote IP address ([\d.]+)/ ) {
$pids{ $pid }{ assigned } = $1;
}
elsif ( /Sent (\d+) bytes, received (\d+) bytes/ ) {
@{ $pids{ $pid } }{ qw/sent rcvd/ } = ( $1, $2 );
}
elsif ( /Connection terminated/ ) {
$pids{ $pid }{ end } = $time;
}
elsif ( /Connect time (.+)/ ) {
$pids{ $pid }{ connect } = $1;
}
}
close FILE;

my $counter;
for my $pid ( sort { $a <=$b } keys %pids ) {
# set empty fields to the string 'n/a'
$_ ||= 'n/a' for @{ $pids{ $pid } }{ qw/ username remote assigned sent
rcvd start end connect / };

$counter++;
print <<STATUS;

Connection $counter
PID: $pid
Username: $pids{$pid}{username}
Remote IP Address: $pids{$pid}{remote}
Assigned IP Address: $pids{$pid}{assigned}
Bytes Sent: $pids{$pid}{sent}
Bytes Received: $pids{$pid}{rcvd}
Start Time: $pids{$pid}{start}
End Time: $pids{$pid}{end}
Duration: $pids{$pid}{connect}

STATUS
}

print <<'MESG';

Please note that any "n/a" entries in the bytes or time columns
may mean that the connection is still up, or that the connection dropped
with an error (such as if the vpn server is restarted.) If there is a "n/a"
in the assigned address field then the connection probably failed. If you can
ping the assigned address then the connection is still up.

Also note that an empty file means there have been no connections today,
you may wish to run this report by hand and specify another date, or you
may wish to view the logs directly.
MESG

__END__

John