remove check_mac to own module, use in macsuck too

Netdisco/lib/App/Netdisco/Core/Arpnip.pm

@@ -3,15 +3,15 @@ package App::Netdisco::Core::Arpnip;
 use Dancer qw/:syntax :script/;
 use Dancer::Plugin::DBIC 'schema';
 
-use App::Netdisco::Util::PortMAC ':all';
+use App::Netdisco::Util::PortMAC 'get_port_macs';
+use App::Netdisco::Util::SanityCheck 'check_mac';
 use App::Netdisco::Util::DNS ':all';
 use NetAddr::IP::Lite ':lower';
 use Time::HiRes 'gettimeofday';
-use Net::MAC;
 
 use base 'Exporter';
 our @EXPORT = ();
-our @EXPORT_OK = qw/ do_arpnip check_mac store_arp /;
+our @EXPORT_OK = qw/ do_arpnip store_arp /;
 our %EXPORT_TAGS = (all => \@EXPORT_OK);
 
 =head1 NAME
@@ -82,91 +82,13 @@ sub _get_arps {
   while (my ($arp, $node) = each %$paddr) {
       my $ip = $netaddr->{$arp};
       next unless defined $ip;
-      push @arps, [$node, $ip, hostname_from_ip($ip)]
+      next unless check_mac($device, $node, $port_macs);
-        if check_mac($device, $node, $port_macs);
+      push @arps, [$node, $ip, hostname_from_ip($ip)];
   }
 
   return @arps;
 }
 
-=head2 check_mac( $device, $node, $port_macs? )
-
-Given a Device database object and a MAC address, perform various sanity
-checks which need to be done before writing an ARP/Neighbor entry to the
-database storage.
-
-Returns false, and logs a debug level message, if the checks fail.
-
-Returns a true value if these checks pass:
-
-=over 4
-
-=item *
-
-MAC address is not malformed
-
-=item *
-
-MAC address is not broadcast, CLIP, VRRP or HSRP
-
-=item *
-
-MAC address does not belong to an interface on C<$device>
-
-=back
-
-Optionally pass a cached set of Device port MAC addresses as the fourth
-argument, or else C<check_mac> will retrieve this for itself from the
-database.
-
-=cut
-
-sub check_mac {
-  my ($device, $node, $port_macs) = @_;
-  $port_macs ||= get_port_macs($device);
-  my $mac = Net::MAC->new(mac => $node, 'die' => 0, verbose => 0);
-
-  # incomplete MAC addresses (BayRS frame relay DLCI, etc)
-  if ($mac->get_error) {
-      debug sprintf ' [%s] arpnip - mac [%s] malformed - skipping',
-        $device->ip, $node;
-      return 0;
-  }
-  else {
-      # lower case, hex, colon delimited, 8-bit groups
-      $node = lc $mac->as_IEEE;
-  }
-
-  # broadcast MAC addresses
-  return 0 if $node eq 'ff:ff:ff:ff:ff:ff';
-
-  # CLIP
-  return 0 if $node eq '00:00:00:00:00:01';
-
-  # VRRP
-  if (index($node, '00:00:5e:00:01:') == 0) {
-      debug sprintf ' [%s] arpnip - VRRP mac [%s] - skipping',
-        $device->ip, $node;
-      return 0;
-  }
-
-  # HSRP
-  if (index($node, '00:00:0c:07:ac:') == 0) {
-      debug sprintf ' [%s] arpnip - HSRP mac [%s] - skipping',
-        $device->ip, $node;
-      return 0;
-  }
-
-  # device's own MACs
-  if (exists $port_macs->{$node}) {
-      debug sprintf ' [%s] arpnip - mac [%s] is device port - skipping',
-        $device->ip, $node;
-      return 0;
-  }
-
-  return 1;
-}
-
 =head2 store_arp( $mac, $ip, $name, $now? )
 
 Stores a new entry to the C<node_ip> table with the given MAC, IP (v4 or v6)

Netdisco/lib/App/Netdisco/Core/Macsuck.pm

@@ -3,7 +3,8 @@ package App::Netdisco::Core::Macsuck;
 use Dancer qw/:syntax :script/;
 use Dancer::Plugin::DBIC 'schema';
 
-use App::Netdisco::Util::PortMAC ':all';
+use App::Netdisco::Util::PortMAC 'get_port_macs';
+use App::Netdisco::Util::SanityCheck 'check_mac';
 use App::Netdisco::Util::SNMP 'snmp_comm_reindex';
 use Time::HiRes 'gettimeofday';
 
@@ -77,9 +78,9 @@ sub do_macsuck {
   }
 
   # now it's time to call store_node for every node discovered
-  # on every port on every vlan on every device.
+  # on every port on every vlan on this device.
 
-  # reverse sort allows vlan 0 entries to be added as fallback
+  # reverse sort allows vlan 0 entries to be included only as fallback
   foreach my $vlan (reverse sort keys %$fwtable) {
       foreach my $port (keys %{ $fwtable->{$vlan} }) {
           if ($device_ports->{$port}->is_uplink) {
@@ -270,6 +271,7 @@ sub _walk_fwtable {
 
   while (my ($idx, $mac) = each %$fw_mac) {
       my $bp_id = $fw_port->{$idx};
+      next unless check_mac($device, $mac, $port_macs);
 
       unless (defined $bp_id) {
           debug sprintf
@@ -361,15 +363,6 @@ sub _walk_fwtable {
           next unless setting('macsuck_bleed');
       }
 
-      if ($mac =~ /^([0-9a-f]{2}):/i and ($1 =~ /.(1|3|5|7|9|b|d|f)/i)) {
-          debug sprintf ' [%s] macsuck %s is multicast - skipping.',
-            $device->ip, $mac;
-          next;
-      }
-
-      next if $mac eq '00:00:00:00:00:00';
-      next if lc($mac) eq 'ff:ff:ff:ff:ff:ff';
-
       ++$cache->{$port}->{$mac};
   }
 

Netdisco/lib/App/Netdisco/Util/SanityCheck.pm

@@ -0,0 +1,115 @@
+package App::Netdisco::Util::SanityCheck;
+
+use Dancer qw/:syntax :script/;
+use Dancer::Plugin::DBIC 'schema';
+
+use App::Netdisco::Util::PortMAC ':all';
+use Net::MAC;
+
+use base 'Exporter';
+our @EXPORT = ();
+our @EXPORT_OK = qw/ check_mac /;
+our %EXPORT_TAGS = (all => \@EXPORT_OK);
+
+=head1 NAME
+
+App::Netdisco::Util::SanityCheck
+
+=head1 DESCRIPTION
+
+Helper subroutines to support parts of the Netdisco application.
+
+There are no default exports, however the C<:all> tag will export all
+subroutines.
+
+=head1 EXPORT_OK
+
+=head2 check_mac( $device, $node, $port_macs? )
+
+Given a Device database object and a MAC address, perform various sanity
+checks which need to be done before writing an ARP/Neighbor entry to the
+database storage.
+
+Returns false, and might log a debug level message, if the checks fail.
+
+Returns a true value if these checks pass:
+
+=over 4
+
+=item *
+
+MAC address is well-formed (according to common formats)
+
+=item *
+
+MAC address is not all-zero, broadcast, CLIP, VRRP or HSRP
+
+=item *
+
+MAC address does not belong to an interface on any known Device
+
+=back
+
+Optionally pass a cached set of Device port MAC addresses as the third
+argument, or else C<check_mac> will retrieve this for itself from the
+database.
+
+=cut
+
+sub check_mac {
+  my ($device, $node, $port_macs) = @_;
+  $port_macs ||= get_port_macs($device);
+  my $mac = Net::MAC->new(mac => $node, 'die' => 0, verbose => 0);
+
+  # incomplete MAC addresses (BayRS frame relay DLCI, etc)
+  if ($mac->get_error) {
+      debug sprintf ' [%s] check_mac - mac [%s] malformed - skipping',
+        $device->ip, $node;
+      return 0;
+  }
+  else {
+      # lower case, hex, colon delimited, 8-bit groups
+      $node = lc $mac->as_IEEE;
+  }
+
+  # broadcast MAC addresses
+  return 0 if $node eq 'ff:ff:ff:ff:ff:ff';
+
+  # all-zero MAC addresses
+  return 0 if $node eq '00:00:00:00:00:00';
+
+  # CLIP
+  return 0 if $node eq '00:00:00:00:00:01';
+
+  # multicast
+  if ($node =~ m/^[0-9a-f](?:1|3|5|7|9|b|d|f):/) {
+      debug sprintf ' [%s] check_mac - multicast mac [%s] - skipping',
+        $device->ip, $node;
+      return 0;
+  }
+
+  # VRRP
+  if (index($node, '00:00:5e:00:01:') == 0) {
+      debug sprintf ' [%s] check_mac - VRRP mac [%s] - skipping',
+        $device->ip, $node;
+      return 0;
+  }
+
+  # HSRP
+  if (index($node, '00:00:0c:07:ac:') == 0) {
+      debug sprintf ' [%s] check_mac - HSRP mac [%s] - skipping',
+        $device->ip, $node;
+      return 0;
+  }
+
+  # device's own MACs
+  if (exists $port_macs->{$node}) {
+      debug sprintf ' [%s] check_mac - mac [%s] is device port - skipping',
+        $device->ip, $node;
+      return 0;
+  }
+
+  return 1;
+}
+
+1;