Perl: Check for free IPs

0.o · 02-12-2009, 11:08 AM

I am trying to write a script to parse a hosts file and pull out missing IP addresses. I am unable to get the logic correct.

Code:

#!/usr/bin/perl

use warnings;
use strict;
my %hosts;

open(HOSTS, "</home/terryd/hosts") or die "Can't open file: $@";
&PopHash;
&FreeIPs;

sub FreeIPs
{
        my $key;
        my $num;
        for $key (keys %hosts)
        {
                foreach(1..254)
                {
                        if(!$hosts{10.4.11.$_})
                        {
                                print "10.4.11.$_ is free\n";
                        }
                }
        }
}

sub PopHash
{#Start of function

        foreach(<HOSTS>)
        {
                chomp;
                next if s/#.*//;
                next if s/^\s+$//;

        if(/10.4.11/)
        {
        {

                my ($ip, $host) = split;

                        if(!$hosts{$host})
                        {
                                $hosts{$ip} = $host;
                        }
        }
        }
}#End of function
close(HOSTS);

Big_Vern · 02-12-2009, 12:26 PM

Quote:

Originally Posted by 0.o

I am trying to write a script to parse a hosts file and pull out missing IP addresses. I am unable to get the logic correct.

Code:

      if(!$hosts{10.4.11.$_})

I can see a few minor issues with your script but to answer your question, you need to quote the beginning of the IP address as the dots are being treated as concatenation operators.

0.o · 02-12-2009, 12:54 PM

Yeah, I figured that out right after posting that. I just removed the first three octets and used the fourth as my key value. Since the addresses are /24 that is all I am concerned about.

What other issues do you see?

Sergei Steshenko · 02-12-2009, 01:51 PM

Code:

if(!$hosts{10.4.11.$_})

- this is really bad style - you create a hash key if it doesn't exist yet.

The correct style is

Code:

unless(exists $hosts${"10.4.11.$_"})

.

chrism01 · 02-12-2009, 05:58 PM

Code:

&PopHash;
&FreeIPs;

The '&'s are unnecessary/deprecated. They are the old school pre Perl v5 way of calling a sub. Now, in most circumstances it means a ref-to-sub, which is not what you mean here.

Also, in sub PopHash you seem to have a doubled set of {} after the if().

In general, if you are using subs, (which is good) I'd pass the hash to & from the subs, not treat it as a global. In bigger progs this can lead to confusion.

In

Code:

open(HOSTS, "</home/terryd/hosts") or die "Can't open file: $@";

use

Code:

open(HOSTS, "<", "/home/terryd/hosts") or die "Can't open file /home/terryd/hosts: $!";

3-arg open is recommended best practice.

Big_Vern · 02-13-2009, 12:12 PM

Quote:

Originally Posted by 0.o

Yeah, I figured that out right after posting that. I just removed the first three octets and used the fourth as my key value. Since the addresses are /24 that is all I am concerned about.

What other issues do you see?

Apart from the few minor things pointed out by the others, I wasn't sure why you were iterating through each element in the hash, then iterating through the numbers 1..254 (or 1..255?). Does this not mean you're checking if an IP exists in the hash, multiple times (depending on the size of the hash)?

0.o · 02-13-2009, 12:20 PM

I changed the script to this:

Code:

#!/usr/bin/perl                                                               
                
use warnings;                                                                 
use strict;                                                                   
my %hosts;                                                                    

open(HOSTS,"<", "/etc/hosts") or die "Can't open file: $@";
&PopHash;                                                  
&FreeIPs;                                                  

sub FreeIPs
{
        foreach(1..254)
        {
                unless(exists $hosts{$_})
                {
                                print "10.4.11.$_ is free\n";

                }
        }
}

sub PopHash
{

        foreach(<HOSTS>)
        {
                chomp;
                next if s/#.*//;
                next if s/^\s+$//;

        if(/10.4.11/)
        {
                my ($ip, $host) = split;
                my($junk1,$junk2,$junk3,$data) = split /\./, $ip;

                unless(exists $hosts{$data})
                {
                        $hosts{$data} = $host;
                }
        }
        }
}
close(HOSTS);
exit(0);

I have changed most things that were mentioned. However, with the use of strict, I am unable to change the way I call the subs. Also, the reason I was iterating through the numbers 1..254 was to check for the existance of that IP. The range of IPs I am checking is a /24 (10.4.11.1-254).

Telemachos · 02-13-2009, 12:38 PM

Quote:

Originally Posted by 0.o

However, with the use of strict, I am unable to change the way I call the subs.

Yes, you can actually:

Code:

FreeIPs();
PopHash();

I would also put the open call inside the subroutine where it runs, and use a scalar file-handle:

Code:

open my $file_handle, '<', '/etc/hosts' or die "Can't open '/etc/hosts' for reading: $!";

Then replace <HOSTS> with <$file_handle>. You can do this as of Perl 5.6, and it's better than bareword filehandles in lots of ways (local to a block, easier to pass around, etc.). http://perldoc.perl.org/perlopentut....ct-Filehandles

Now that I look, you also don't want to substitute in your regular expressions in PopHash - you only want to match. If you're skipping the item anyhow, why perform any substitution? (You're never going to process that line, and you're not writing the file back out after editing it. So why bother?):

Code:

next if m/^#/;      # Skip lines that are only comments
next if m/^\s+$/;   # Skip lines that are blank

You might also use a slice rather than assign three dummy values in that same subroutine:

Code:

my $data = (split /\./, $ip)[3];