2005-02-08 Andrew J. Schorr <ajschorr@alumni.princeton.edu>

	* ospf_interface.h: Improve passive_interface comment.  Add new
	  multicast_memberships bitmask to struct ospf_interface to track
	  active multicast subscriptions.  Declare new function
	  ospf_if_set_multicast.
	* ospf_interface.c: (ospf_if_set_multicast) New function to configure
	  multicast memberships properly based on the current
	  multicast_memberships status and the current values of the
	  ospf_interface state, type, and passive_interface status.
	  (ospf_if_up) Remove call to ospf_if_add_allspfrouters (this is
	  now handled by ism_change_state's call to ospf_if_set_multicast).
	  (ospf_if_down) Remove call to ospf_if_drop_allspfrouters (now
	  handled by ism_change_state).
	* ospf_ism.c: (ospf_dr_election) Remove logic to join or leave
	  the DRouters multicast group (now handled by ism_change_state's call
	  to ospf_if_set_multicast).
	  (ism_change_state) Add call to ospf_if_set_multicast to change
	  multicast memberships as necessary to reflect the new interface state.
	* ospf_packet.c: (ospf_hello) When a Hello packet is received on a
	  passive interface: 1. Increase the severity of the error message
	  from LOG_INFO to LOG_WARNING; 2. Add more information to the error
	  message (packet destination address and interface address);
	  and 3. If the packet was sent to ospf-all-routers, then try
	  to fix the multicast group memberships.
	  (ospf_read) When a packet is received on an interface whose state
	  is ISM_Down, enhance the warning message to show the packet
	  destination address, and try to update/fix the multicast group
	  memberships if the packet was sent to a multicast address.
	  When a packet is received for ospf-designated-routers, but the
	  current interface state is not DR or BDR, then increase the
	  severity level of the error message from LOG_INFO to LOG_WARNING,
	  and try to fix the multicast group memberships.
	* ospf_vty.c: (ospf_passive_interface) Call ospf_if_set_multicast for
	  any ospf interface that may have changed from active to passive.
	  (no_ospf_passive_interface) Call ospf_if_set_multicast for
	  any ospf interface that may have changed from passive to active.
	  (show_ip_ospf_interface_sub) Show multicast group memberships.

6 files changed, 186 insertions, 24 deletions

diff --git a/ospfd/ChangeLog b/ospfd/ChangeLog
index ba418ba5..b9e21c5b 100644
--- a/ospfd/ChangeLog
+++ b/ospfd/ChangeLog
@@ -1,3 +1,42 @@
+2005-02-08 Andrew J. Schorr <ajschorr@alumni.princeton.edu>
+
+	* ospf_interface.h: Improve passive_interface comment.  Add new
+	  multicast_memberships bitmask to struct ospf_interface to track
+	  active multicast subscriptions.  Declare new function
+	  ospf_if_set_multicast.
+	* ospf_interface.c: (ospf_if_set_multicast) New function to configure
+	  multicast memberships properly based on the current
+	  multicast_memberships status and the current values of the
+	  ospf_interface state, type, and passive_interface status.
+	  (ospf_if_up) Remove call to ospf_if_add_allspfrouters (this is
+	  now handled by ism_change_state's call to ospf_if_set_multicast).
+	  (ospf_if_down) Remove call to ospf_if_drop_allspfrouters (now
+	  handled by ism_change_state).
+	* ospf_ism.c: (ospf_dr_election) Remove logic to join or leave
+	  the DRouters multicast group (now handled by ism_change_state's call
+	  to ospf_if_set_multicast).
+	  (ism_change_state) Add call to ospf_if_set_multicast to change
+	  multicast memberships as necessary to reflect the new interface state.
+	* ospf_packet.c: (ospf_hello) When a Hello packet is received on a
+	  passive interface: 1. Increase the severity of the error message
+	  from LOG_INFO to LOG_WARNING; 2. Add more information to the error
+	  message (packet destination address and interface address);
+	  and 3. If the packet was sent to ospf-all-routers, then try
+	  to fix the multicast group memberships.
+	  (ospf_read) When a packet is received on an interface whose state
+	  is ISM_Down, enhance the warning message to show the packet
+	  destination address, and try to update/fix the multicast group
+	  memberships if the packet was sent to a multicast address.
+	  When a packet is received for ospf-designated-routers, but the
+	  current interface state is not DR or BDR, then increase the
+	  severity level of the error message from LOG_INFO to LOG_WARNING,
+	  and try to fix the multicast group memberships.
+	* ospf_vty.c: (ospf_passive_interface) Call ospf_if_set_multicast for
+	  any ospf interface that may have changed from active to passive.
+	  (no_ospf_passive_interface) Call ospf_if_set_multicast for
+	  any ospf interface that may have changed from passive to active.
+	  (show_ip_ospf_interface_sub) Show multicast group memberships.
+
 2005-02-08 Paul Jakma <paul@dishone.st>
 
 	* ospf_packet.c: (various) Remove unneeded stream_set_putp abuse.
diff --git a/ospfd/ospf_interface.c b/ospfd/ospf_interface.c
index da08360d..df71fad6 100644
--- a/ospfd/ospf_interface.c
+++ b/ospfd/ospf_interface.c
@@ -727,6 +727,60 @@ ospf_if_is_enable (struct ospf_interface *oi)
   return 0;
 }
 
+void
+ospf_if_set_multicast(struct ospf_interface *oi)
+{
+  if ((oi->state > ISM_Loopback) &&
+      (oi->type != OSPF_IFTYPE_LOOPBACK) &&
+      (oi->type != OSPF_IFTYPE_VIRTUALLINK) &&
+      (OSPF_IF_PARAM(oi, passive_interface) == OSPF_IF_ACTIVE))
+    {
+      /* The interface should belong to the OSPF-all-routers group. */
+      if (!CHECK_FLAG(oi->multicast_memberships, MEMBER_ALLROUTERS) &&
+	  (ospf_if_add_allspfrouters(oi->ospf, oi->address,
+				     oi->ifp->ifindex) >= 0))
+	/* Set the flag only if the system call to join succeeded. */
+        SET_FLAG(oi->multicast_memberships, MEMBER_ALLROUTERS);
+    }
+  else
+    {
+      /* The interface should NOT belong to the OSPF-all-routers group. */
+      if (CHECK_FLAG(oi->multicast_memberships, MEMBER_ALLROUTERS))
+        {
+	  ospf_if_drop_allspfrouters (oi->ospf, oi->address, oi->ifp->ifindex);
+	  /* Unset the flag regardless of whether the system call to leave
+	     the group succeeded, since it's much safer to assume that
+	     we are not a member. */
+	  UNSET_FLAG(oi->multicast_memberships, MEMBER_ALLROUTERS);
+        }
+    }
+
+  if (((oi->type == OSPF_IFTYPE_BROADCAST) ||
+       (oi->type == OSPF_IFTYPE_POINTOPOINT)) &&
+      ((oi->state == ISM_DR) || (oi->state == ISM_Backup)) &&
+      (OSPF_IF_PARAM(oi, passive_interface) == OSPF_IF_ACTIVE))
+    {
+      /* The interface should belong to the OSPF-designated-routers group. */
+      if (!CHECK_FLAG(oi->multicast_memberships, MEMBER_DROUTERS) &&
+	  (ospf_if_add_alldrouters(oi->ospf, oi->address,
+	  			   oi->ifp->ifindex) >= 0))
+	/* Set the flag only if the system call to join succeeded. */
+        SET_FLAG(oi->multicast_memberships, MEMBER_DROUTERS);
+    }
+  else
+    {
+      /* The interface should NOT belong to the OSPF-designated-routers group */
+      if (CHECK_FLAG(oi->multicast_memberships, MEMBER_DROUTERS))
+        {
+	  ospf_if_drop_alldrouters(oi->ospf, oi->address, oi->ifp->ifindex);
+	  /* Unset the flag regardless of whether the system call to leave
+	     the group succeeded, since it's much safer to assume that
+	     we are not a member. */
+          UNSET_FLAG(oi->multicast_memberships, MEMBER_DROUTERS);
+        }
+    }
+}
+
 int
 ospf_if_up (struct ospf_interface *oi)
 {
@@ -737,8 +791,6 @@ ospf_if_up (struct ospf_interface *oi)
     OSPF_ISM_EVENT_SCHEDULE (oi, ISM_LoopInd);
   else
     {
-      if (oi->type != OSPF_IFTYPE_VIRTUALLINK)
-	ospf_if_add_allspfrouters (oi->ospf, oi->address, oi->ifp->ifindex);
       ospf_if_stream_set (oi);
       OSPF_ISM_EVENT_SCHEDULE (oi, ISM_InterfaceUp);
     }
@@ -755,9 +807,6 @@ ospf_if_down (struct ospf_interface *oi)
   OSPF_ISM_EVENT_EXECUTE (oi, ISM_InterfaceDown);
   /* Shutdown packet reception and sending */
   ospf_if_stream_unset (oi);
-  if (oi->type != OSPF_IFTYPE_VIRTUALLINK)
-    ospf_if_drop_allspfrouters (oi->ospf, oi->address, oi->ifp->ifindex);
-
 
   return 1;
 }
diff --git a/ospfd/ospf_interface.h b/ospfd/ospf_interface.h
index 60ee1650..ca22c1a0 100644
--- a/ospfd/ospf_interface.h
+++ b/ospfd/ospf_interface.h
@@ -44,7 +44,7 @@ struct ospf_if_params
   DECLARE_IF_PARAM (u_int32_t, transmit_delay); /* Interface Transmisson Delay */
   DECLARE_IF_PARAM (u_int32_t, output_cost_cmd);/* Command Interface Output Cost */
   DECLARE_IF_PARAM (u_int32_t, retransmit_interval); /* Retransmission Interval */
-  DECLARE_IF_PARAM (u_char, passive_interface);      /* OSPF Interface is passive */
+  DECLARE_IF_PARAM (u_char, passive_interface);      /* OSPF Interface is passive: no sending or receiving (no need to join multicast groups) */
   DECLARE_IF_PARAM (u_char, priority);               /* OSPF Interface priority */
   DECLARE_IF_PARAM (u_char, type);                   /* type of interface */
 #define OSPF_IF_ACTIVE                  0
@@ -126,6 +126,11 @@ struct ospf_interface
   struct prefix *address;		/* Interface prefix */
   struct connected *connected;          /* Pointer to connected */ 
 
+  /* To which multicast groups do we currently belong? */
+  u_char multicast_memberships;
+#define MEMBER_ALLROUTERS	0x1
+#define MEMBER_DROUTERS		0x2
+
   /* Configured varables. */
   struct ospf_if_params *params;
   u_int32_t crypt_seqnum;		/* Cryptographic Sequence Number */ 
@@ -248,4 +253,8 @@ int ospf_crypt_key_delete (struct list *, u_char);
 
 u_char ospf_default_iftype(struct interface *ifp);
 
+/* Set all multicast memberships appropriately based on the type and
+   state of the interface. */
+extern void ospf_if_set_multicast(struct ospf_interface *);
+
 #endif /* _ZEBRA_OSPF_INTERFACE_H */
diff --git a/ospfd/ospf_ism.c b/ospfd/ospf_ism.c
index 17ec9b5a..dd0f0669 100644
--- a/ospfd/ospf_ism.c
+++ b/ospfd/ospf_ism.c
@@ -254,17 +254,6 @@ ospf_dr_election (struct ospf_interface *oi)
       !IPV4_ADDR_SAME (&old_bdr, &BDR (oi)))
     ospf_dr_change (oi->ospf, oi->nbrs);
 
-  if (oi->type == OSPF_IFTYPE_BROADCAST || oi->type == OSPF_IFTYPE_POINTOPOINT)
-    {
-      /* Multicast group change. */
-      if ((old_state != ISM_DR && old_state != ISM_Backup) &&
-	  (new_state == ISM_DR || new_state == ISM_Backup))
-	ospf_if_add_alldrouters (oi->ospf, oi->address, oi->ifp->ifindex);
-      else if ((old_state == ISM_DR || old_state == ISM_Backup) &&
-	       (new_state != ISM_DR && new_state != ISM_Backup))
-	ospf_if_drop_alldrouters (oi->ospf, oi->address, oi->ifp->ifindex);
-    }
-
   return new_state;
 }
 
@@ -581,6 +570,9 @@ ism_change_state (struct ospf_interface *oi, int state)
   oi->state = state;
   oi->state_change++;
 
+  /* Set multicast memberships appropriately for new state. */
+  ospf_if_set_multicast(oi);
+
   if (old_state == ISM_Down || state == ISM_Down)
     ospf_check_abr_status (oi->ospf);
 
diff --git a/ospfd/ospf_packet.c b/ospfd/ospf_packet.c
index 341c31bd..4700329e 100644
--- a/ospfd/ospf_packet.c
+++ b/ospfd/ospf_packet.c
@@ -774,8 +774,19 @@ ospf_hello (struct ip *iph, struct ospf_header *ospfh,
 
   /* If incoming interface is passive one, ignore Hello. */
   if (OSPF_IF_PARAM (oi, passive_interface) == OSPF_IF_PASSIVE) {
-    zlog_info ("Packet %s [HELLO:RECV]: oi is passive",
-               inet_ntoa (ospfh->router_id));
+    char buf[3][INET_ADDRSTRLEN];
+    zlog_warn("Warning: ignoring HELLO from router %s sent to %s; we "
+	      "should not receive hellos on passive interface %s!",
+	      inet_ntop(AF_INET, &ospfh->router_id, buf[0], sizeof(buf[0])),
+	      inet_ntop(AF_INET, &iph->ip_dst, buf[1], sizeof(buf[1])),
+	      inet_ntop(AF_INET, &oi->address->u.prefix4,
+	      		buf[2], sizeof(buf[2])));
+    if (iph->ip_dst.s_addr == htonl(OSPF_ALLSPFROUTERS))
+      {
+        /* Try to fix multicast membership. */
+        SET_FLAG(oi->multicast_memberships, MEMBER_ALLROUTERS);
+        ospf_if_set_multicast(oi);
+      }
     return;
   }
 
@@ -2428,10 +2439,20 @@ ospf_read (struct thread *thread)
     }
   else if (oi->state == ISM_Down)
     {
-      zlog_warn ("Ignoring packet from [%s] received on interface that is "
+      char buf[2][INET_ADDRSTRLEN];
+      zlog_warn ("Ignoring packet from %s to %s received on interface that is "
       		 "down [%s]; interface flags are %s",
-                 inet_ntoa (iph->ip_src), ifp->name, if_flag_dump(ifp->flags));
+		 inet_ntop(AF_INET, &iph->ip_src, buf[0], sizeof(buf[0])),
+		 inet_ntop(AF_INET, &iph->ip_dst, buf[1], sizeof(buf[1])),
+	         ifp->name, if_flag_dump(ifp->flags));
       stream_free (ibuf);
+      /* Fix multicast memberships? */
+      if (iph->ip_dst.s_addr == htonl(OSPF_ALLSPFROUTERS))
+	SET_FLAG(oi->multicast_memberships, MEMBER_ALLROUTERS);
+      else if (iph->ip_dst.s_addr == htonl(OSPF_ALLDROUTERS))
+	SET_FLAG(oi->multicast_memberships, MEMBER_DROUTERS);
+      if (oi->multicast_memberships)
+	ospf_if_set_multicast(oi);
       return 0;
     }
 
@@ -2443,10 +2464,13 @@ ospf_read (struct thread *thread)
   if (iph->ip_dst.s_addr == htonl (OSPF_ALLDROUTERS)
   && (oi->state != ISM_DR && oi->state != ISM_Backup))
     {
-      zlog_info ("Packet for AllDRouters from [%s] via [%s] (ISM: %s)",
+      zlog_warn ("Dropping packet for AllDRouters from [%s] via [%s] (ISM: %s)",
                  inet_ntoa (iph->ip_src), IF_NAME (oi),
                  LOOKUP (ospf_ism_state_msg, oi->state));
       stream_free (ibuf);
+      /* Try to fix multicast membership. */
+      SET_FLAG(oi->multicast_memberships, MEMBER_DROUTERS);
+      ospf_if_set_multicast(oi);
       return 0;
     }
 
diff --git a/ospfd/ospf_vty.c b/ospfd/ospf_vty.c
index b4c12ffb..e3f8f1b2 100644
--- a/ospfd/ospf_vty.c
+++ b/ospfd/ospf_vty.c
@@ -260,6 +260,7 @@ DEFUN (ospf_passive_interface,
  struct in_addr addr;
  int ret;
  struct ospf_if_params *params;
+ struct route_node *rn;
 
  ifp = if_lookup_by_name (argv[0]);
  
@@ -287,7 +288,28 @@ DEFUN (ospf_passive_interface,
 
   SET_IF_PARAM (params, passive_interface);
   params->passive_interface = OSPF_IF_PASSIVE;
- 
+
+  /* XXX We should call ospf_if_set_multicast on exactly those
+   * interfaces for which the passive property changed.  It is too much
+   * work to determine this set, so we do this for every interface.
+   * This is safe and reasonable because ospf_if_set_multicast uses a
+   * record of joined groups to avoid systems calls if the desired
+   * memberships match the current memership.
+   */
+  for (rn = route_top(IF_OIFS(ifp)); rn; rn = route_next (rn))
+    {
+      struct ospf_interface *oi = rn->info;
+
+      if (oi && (OSPF_IF_PARAM(oi, passive_interface) == OSPF_IF_PASSIVE))
+        ospf_if_set_multicast(oi);
+    }
+  /*
+   * XXX It is not clear what state transitions the interface needs to
+   * undergo when going from active to passive.  Fixing this will
+   * require precise identification of interfaces having such a
+   * transition.
+   */
+
  return CMD_SUCCESS;
 }
 
@@ -308,6 +330,7 @@ DEFUN (no_ospf_passive_interface,
   struct in_addr addr;
   struct ospf_if_params *params;
   int ret;
+  struct route_node *rn;
     
   ifp = if_lookup_by_name (argv[0]);
   
@@ -342,7 +365,22 @@ DEFUN (no_ospf_passive_interface,
       ospf_free_if_params (ifp, addr);
       ospf_if_update_params (ifp, addr);
     }
-  
+
+  /* XXX We should call ospf_if_set_multicast on exactly those
+   * interfaces for which the passive property changed.  It is too much
+   * work to determine this set, so we do this for every interface.
+   * This is safe and reasonable because ospf_if_set_multicast uses a
+   * record of joined groups to avoid systems calls if the desired
+   * memberships match the current memership.
+   */
+  for (rn = route_top(IF_OIFS(ifp)); rn; rn = route_next (rn))
+    {
+      struct ospf_interface *oi = rn->info;
+
+      if (oi && (OSPF_IF_PARAM(oi, passive_interface) == OSPF_IF_ACTIVE))
+        ospf_if_set_multicast(oi);
+    }
+
   return CMD_SUCCESS;
 }
 
@@ -2628,6 +2666,17 @@ show_ip_ospf_interface_sub (struct vty *vty, struct ospf *ospf,
 		       inet_ntoa (nbr->address.u.prefix4), VTY_NEWLINE);
 	    }
 	}
+
+      vty_out (vty, "  Multicast group memberships:");
+      if (CHECK_FLAG(oi->multicast_memberships, MEMBER_ALLROUTERS))
+        vty_out (vty, " OSPFAllRouters");
+      if (CHECK_FLAG(oi->multicast_memberships, MEMBER_DROUTERS))
+        vty_out (vty, " OSPFDesignatedRouters");
+      if (!CHECK_FLAG(oi->multicast_memberships,
+		      MEMBER_ALLROUTERS|MEMBER_DROUTERS))
+        vty_out (vty, " <None>");
+      vty_out (vty, "%s", VTY_NEWLINE);
+
       vty_out (vty, "  Timer intervals configured,");
       vty_out (vty, " Hello %d, Dead %d, Wait %d, Retransmit %d%s",
 	       OSPF_IF_PARAM (oi, v_hello), OSPF_IF_PARAM (oi, v_wait),