23
\$\begingroup\$

A while ago I was experiencing 'flakiness' on my home ADSL line. I run a Linux server in my basement, VOIP, and a few other things (like Netflix) and I was experiencing a few minutes of network downtime every few hours. This was dropping very important things, like the kids watching cartoons, and my phone-calls with my parents.

In communication with my ISP (who are fantastic, I'm not complaining), they wondered how often the connection was dropping, and I could not easily say... so I wrote a script to monitor....

All it does is ping the remote side of my ADSL line every second (my gateway). This is a short hop, and not much of an impact.

Here's the script I wrote, and, based on it, I was able to escalate the problem, and copy relevant mails to the ISP, and they were able to correlate the issue to problems somewhere... which ended up with me replacing my ADSL modem, and problem solved.

Long story, for some short code, but, this is a classic perl hack...

My perl is somewhat old-school, so I am looking for ideas on how to modernize my usage, and any other tips and tricks.

#!/usr/bin/perl

use strict;
use warnings;
use autodie;

use Net::Ping;
use Mail::Sendmail;

$| = 1;

my $defaultmail = '[email protected]';
my $defaultip = "8.8.8.8"; # Google's nameserver... 

my $host = shift @ARGV;
$host ||= $defaultip;

my $mailto = shift @ARGV;
$mailto ||= $defaultmail;

sub mail {

  my $subj = shift;
  my $msg = scalar(localtime) . " -> " . $subj;
  my %mail;

  %mail = (smtp    => 'localhost:25',
           To      => $mailto,
           From    => ,$mailto,
           'X-InternetUp' => "$0",
           Subject => $subj,
           Message => $msg);

  sendmail(%mail) or die;
}


my $ping = Net::Ping->new();

my $up = 0; # start with it down, and it sends an 'up mail'.
my $start = time();

mail ("Starting network monitor");

while (1) {
  sleep 1;

  if ($ping->ping($host)) {
    if (!$up) {
      my $interval = time() - $start;
      $start += $interval;
      mail(sprintf("INTERNET -> JUST COME BACK - Was DOWN for %d seconds\n", $interval));
    }
    $up = 1;
  } else {
    if ($up) {
      my $interval = time() - $start;
      $start += $interval;
      mail(sprintf("INTERNET -> JUST GONE DOWN - Was UP for %d seconds\n", $interval));
    }
    $up = 0;

  }

}

This script leaves mails like:

Subject: INTERNET -> JUST GONE DOWN - Was UP for 65174 seconds
X-Internetup: ./netup
Date: Thu, 23 Oct 2014 14:07:23 -0400
To: rolfl@....
Content-Transfer-Encoding: quoted-printable
From: rolfl@...
Message-Id: <20141023180723.85C01440E60@mail....>

Thu Oct 23 14:07:23 2014 -> INTERNET -> JUST GONE DOWN - Was UP for 65174 s=
econds
\$\endgroup\$

1 Answer 1

11
\$\begingroup\$

Argument handling

$defaultmail and $defaultip are poorly named — there is a naming inconsistency:

$host ||= $defaultip;
…
$mailto ||= $defaultmail;

I would also advise against introducing $defaultmail and $defaultip as variables at all. Either just write them in directly:

my $host   = (shift @ARGV) || '8.8.8.8';            # Google's nameserver
my $mailto = (shift @ARGV) || '[email protected]';  # RFC 2606

or define a constant:

use constant DEFAULTS => {
    host   => '8.8.8.8',              # Google's nameserver
    mailto => '[email protected]',
};

my $host   = (shift @ARGV) // DEFAULTS->{host};
my $mailto = (shift @ARGV) // DEFAULTS->{mailto};

Here, I've modernized things by using the logical defined-or operator //, introduced in Perl 5.10. That lets falsy strings like '0' be treated as a valid parameter.

Ideally, the mail subroutine should not be a closure that captures the value of $mailto. I suppose such sloppiness is acceptable for a quick hack like this.

Mail

Mail is a funny choice as a logging medium, I think. If your Internet connection goes down, the message will just get stuck in your local MTA's queue for a while. Appending to a text file would work, and the results would be easier to analyze.

Also for ease of analysis, I recommend using a timestamp format based on ISO 8601.

Public service announcement: This is THE correct way to write numeric dates: 2013-02-27

There is a stray comma in the assignment of %mail for the From value. I'm surprised that the script works anyway. It just goes to show that "Only perl can parse Perl".

While it is acceptable to define any mail header you want whose name starts with X-…, the X-Mailer header is a de-facto standard that would have been applicable.

I wouldn't bother defining %mail as a variable. Just call sendmail directly, and make them look like named parameters.

Ping

Net::Ping defaults to using the TCP echo protocol, which is rarely supported. It certainly isn't supported by Google's 8.8.8.8 DNS server, so your defaults don't work.

You could use an ICMP probe, but that would require root privileges on Unix.

A TCP probe to 8.8.8.8:53 (DNS over TCP) seems to work. It would also be reasonable to expect your ISP's nameserver to respond to DNS over TCP. (If not, your Internet service would be effectively down anyway.)

Loop

I'm not a fan of while (1) { sleep 1; … } loops. I suggest writing it as do { … } while sleep 1;.

You have some repeated code inside the two branches. I think you have written the conditions suboptimally. What you really want is to act on state transitions, so write it that way.

Suggested implementation

#!/usr/bin/perl

use strict;
use warnings;
use autodie;

use Mail::Sendmail;
use Net::Ping;
use POSIX qw(strftime);

use constant DEFAULTS => {
    host   => '8.8.8.8',              # Google's nameserver
    mailto => '[email protected]',
};

my $host   = (shift @ARGV) // DEFAULTS->{host};
my $mailto = (shift @ARGV) // DEFAULTS->{mailto};

sub mail {
    my $subj = shift;
    my $msg = strftime('%FT%T', localtime) . " -> " . $subj;

    sendmail(smtp    => 'localhost:25',
             To      => $mailto,
             From    => $mailto,
             'X-Mailer' => "InternetUp $0",
             Subject => $subj,
             Message => $msg) or die;
}


# $| = 1; # <-- The script prints nothing to STDOUT.  This is pointless.

my $ping = Net::Ping->new('tcp');
$ping->port_number(getservbyname('domain', 'tcp'));

my $up = 0; # start with it down, and it sends an 'up mail'.
my $start = time;

mail("Starting network monitor");

do {
    my $ping_ok = $ping->ping($host);
    if ($ping_ok != $up) {
        $up = $ping_ok;

        my $interval = time - $start;
        $start += $interval;
        mail(sprintf("INTERNET -> %s for %d seconds\n",
                     $up ? "JUST CAME BACK - Was DOWN"
                         : "JUST GONE DOWN - Was UP",
                     $interval));
    }
} while sleep 1;
\$\endgroup\$
1
  • 2
    \$\begingroup\$ @ChrisWue The format specifiers for POSIX::strftime() are admittedly system dependent: "If you want your code to be portable, your format (fmt) argument should only use the format specifiers defined by the ANSI C standard." You could use POSIX::strftime::GNU to avoid relying on system library support. \$\endgroup\$ Oct 27, 2014 at 21:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.