2004-01-05 Greg Troxel <gdt@fnord.ir.bbn.com>

        * kernel_socket.c (ifm_read): Major cleanup.  Use Sowmini's code
        to find the sockaddr_dl in all cases, narrowing the Solaris ifdef
        to just the accomodation of broken kernels.  Check sockaddr_dl
        carefully up front, and later assume any non-NULL sdl pointer is
        valid.  Clean up types and variable declarations, and rename
        WRAPUP to SAROUNDUP to make the name fit the behavior.

2 files changed, 77 insertions, 94 deletions

diff --git a/zebra/ChangeLog b/zebra/ChangeLog
index c346ce1a..945fa57b 100644
--- a/zebra/ChangeLog
+++ b/zebra/ChangeLog
@@ -1,10 +1,18 @@
 2004-01-05  Greg Troxel  <gdt@fnord.ir.bbn.com>
+	* kernel_socket.c (ifm_read): Major cleanup.  Use Sowmini's code
+	to find the sockaddr_dl in all cases, narrowing the Solaris ifdef
+	to just the accomodation of broken kernels.  Check sockaddr_dl
+	carefully up front, and later assume any non-NULL sdl pointer is
+	valid.  Clean up types and variable declarations, and rename
+	WRAPUP to SAROUNDUP to make the name fit the behavior.
+
+2004-01-05  Greg Troxel  <gdt@fnord.ir.bbn.com>
 
 	* kernel_socket.c (kernel_read): Add a sockaddr_dl to the ifmsg
 	structure, because on Solaris sockaddr_dl is far larger than the
 	base sockaddr structure.  (The code had previously been failing to
 	read all the data.)
-
+	
 2004-01-05  Greg Troxel  <gdt@ahi.ir.bbn.com>
 
 	* kernel_socket.c (kernel_read): Look up interfaces by index
diff --git a/zebra/kernel_socket.c b/zebra/kernel_socket.c
index 59bb023b..b8dfcc7d 100644
--- a/zebra/kernel_socket.c
+++ b/zebra/kernel_socket.c
@@ -40,15 +40,29 @@
 extern struct zebra_privs_t zserv_privs;
 extern struct zebra_t zebrad;
 
-/* Socket length roundup function. */
+/*
+ * Given a sockaddr length, round it up to include pad bytes following
+ * it.  Assumes the kernel pads to sizeof(long).
+ *
+ * XXX: why is ROUNDUP(0) sizeof(long)?  0 is an illegal sockaddr
+ * length anyway (< sizeof (struct sockaddr)), so this shouldn't
+ * matter.
+ */
 #define ROUNDUP(a) \
   ((a) > 0 ? (1 + (((a) - 1) | (sizeof(long) - 1))) : sizeof(long))
 
-/* And this macro is wrapper for handling sa_len. */
+/*
+ * Given a pointer (sockaddr or void *), return the number of bytes
+ * taken up by the sockaddr and any padding needed for alignment.
+ */
 #if defined(HAVE_SA_LEN)
-#define WRAPUP(X)   ROUNDUP(((struct sockaddr *)(X))->sa_len)
+#define SAROUNDUP(X)   ROUNDUP(((struct sockaddr *)(X))->sa_len)
 #elif defined(HAVE_IPV6)
-#define WRAPUP(X) \
+/*
+ * One would hope all fixed-size structure definitions are aligned,
+ * but round them up nonetheless.
+ */
+#define SAROUNDUP(X) \
     do { \
     (((struct sockaddr *)(X))->sa_family == AF_INET ?   \
       ROUNDUP(sizeof(struct sockaddr_in)):\
@@ -58,7 +72,7 @@ extern struct zebra_t zebrad;
          ROUNDUP(sizeof(struct sockaddr_dl)) : sizeof(struct sockaddr)))) \
     } while (0)
 #else /* HAVE_IPV6 */ 
-#define WRAPUP(X) \
+#define SAROUNDUP(X) \
       (((struct sockaddr *)(X))->sa_family == AF_INET ?   \
         ROUNDUP(sizeof(struct sockaddr_in)):\
          (((struct sockaddr *)(X))->sa_family == AF_LINK ? \
@@ -221,6 +235,8 @@ ifm_read (struct if_msghdr *ifm)
 {
   struct interface *ifp = NULL;
   struct sockaddr_dl *sdl = NULL;
+  void *cp;
+  unsigned int i;
   char ifname[IFNAMSIZ];
 
   /* paranoia: sanity check structure */
@@ -232,91 +248,55 @@ ifm_read (struct if_msghdr *ifm)
     }
 
   /*
-   * Check for a sockaddr_dl following the message.
+   * Check for a sockaddr_dl following the message.  First, point to
+   * where a socakddr might be if one follows the message.
    */
-#ifdef SUNOS_5
-  int i;
-  struct sockaddr *sa;
-  u_char *cp = (u_char *)(ifm + 1);
+  cp = (void *)(ifm + 1);
 
+#ifdef SUNOS_5
   /* 
+   * XXX This behavior should be narrowed to only the kernel versions
+   * for which the structures returned do not match the headers.
+   *
    * if_msghdr_t on 64 bit kernels in Solaris 9 and earlier versions
-   * is 12 bytes larger than the 32 bit version, so make adjustment
-   * here.
+   * is 12 bytes larger than the 32 bit version.
    */
-  sa = (struct sockaddr *)cp;
-  if (sa->sa_family == AF_UNSPEC)
+  if (((struct sockaddr *) cp)->sa_family == AF_UNSPEC)
   	cp = cp + 12;
+#endif
 
+  /* 
+   * Check for each sockaddr in turn, advancing over it.  After this
+   * loop, sdl should point to a sockaddr_dl iff one was present.
+   */
   for (i = 1; i != 0; i <<= 1) 
     {
-      if (i & ifm->ifm_addrs) 
+      if (i & ifm->ifm_addrs)
         {
-          sa = (struct sockaddr *)cp;
-          cp += WRAPUP(sa);
-	  if (i & RTA_IFP)
+	  if (i == RTA_IFP)
 	    {
-	      sdl = (struct sockaddr_dl *)sa;
+	      sdl = (struct sockaddr_dl *)cp;
 	      break;
             }
+	  cp += SAROUNDUP(cp);
         }
     }
-  /*
-   * After here, If RTA_IFP was set in ifm_addrs, sdl should point to
-   * the sockaddr_dl.
-   */
-#else
-  /* sockaddrs_present? */
-  if (ifm->ifm_addrs)
+
+  /* Ensure that sdl, if present, is actually a sockaddr_dl. */
+  if (sdl != NULL && sdl->sdl_family != AF_LINK)
     {
-      if (ifm->ifm_addrs == RTA_IFP)
-	{
-	  /* just the one we want */
-	  sdl = (struct sockaddr_dl *)(ifm + 1);
-	}
-      else
-	{
-	  /*
-	   * Not strictly an error, but more complicated parsing than
-	   * is implemented is required to handle this case.
-	   */
-	  zlog_err ("ifm_read: addrs %x != RTA_IFP (unhandled, ignoring)\n",
-		    ifm->ifm_addrs);
-	  return -1;
-      }
+      zlog_err ("ifm_read: sockaddr_dl bad AF %d\n",
+		sdl->sdl_family);
+      return -1;
     }
-  /*
-   * Past here, either ifm_addrs == 0 or ifm_addrs == RTA_IFP and sdl
-   * points to a RTA_IFP sockaddr.
-   */
-#endif
-
-  /* Check that sdl, if present, is actually a sockaddr_dl before use. */
-  if (sdl != NULL)
-    switch (sdl->sdl_family)
-      {
-      case AF_LINK:
-	/* Standard AF for link-layer address sockaddrs. */
-      case AF_DLI:
-	/*
-	 * XXX Comment in NetBSD net/if_dl.h says AF_DLI, but this
-	 * seems wrong.  Accept it for now.
-	 */
-	break;
-
-      default:
-	zlog_err ("ifm_read: sockaddr_dl bad AF %d\n",
-		  sdl->sdl_family);
-	return -1;
-      }
 
   /* 
-   * Look up on ifindex.  This is useful if this is an up/down
-   * notification for an interface of which we are already aware.
-   * (This happens on NetBSD 1.6.2, for example.)
+   * Look up on ifindex first, because ifindices are the primary
+   * handle for interfaces across the user/kernel boundary.  (Some
+   * messages, such as up/down status changes on NetBSD, do not
+   * include a sockaddr_dl).
    */
-  if (ifp == NULL)
-    ifp = if_lookup_by_index (ifm->ifm_index);
+  ifp = if_lookup_by_index (ifm->ifm_index);
 
   /* 
    * If lookup by index was unsuccessful and we have a name, try
@@ -348,28 +328,25 @@ ifm_read (struct if_msghdr *ifm)
    */
   if ((ifp == NULL) || (ifp->ifindex == -1))
     {
-      /* Check interface's address.*/
-      if (! (ifm->ifm_addrs & RTA_IFP))
-	{
-	  zlog_warn ("Interface index %d (new) missing RTA_IFP sockaddr\n",
-		     ifm->ifm_index);
-	  return -1;
-	}
-
       /*
-       * paranoia: sdl-finding code above guarantees that sdl is
-       * non-NULL and valid if RTA_IFP is set in ifm_addrs, so this
-       * check is in theory not necessary.
+       * To create or fill in an interface, a sockaddr_dl (via
+       * RTA_IFP) is required.
        */
       if (sdl == NULL)
 	{
-	  zlog_warn ("ifm_read: no sockaddr_dl present");
+	  zlog_warn ("Interface index %d (new) missing RTA_IFP sockaddr_dl\n",
+		     ifm->ifm_index);
 	  return -1;
 	}
 
       if (ifp == NULL)
+	/* Interface that zebra was not previously aware of, so create. */ 
       	ifp = if_create (sdl->sdl_data, sdl->sdl_nlen);
 
+      /* 
+       * Fill in newly created interface structure, or larval
+       * structure with ifindex -1.
+       */
       ifp->ifindex = ifm->ifm_index;
       ifp->flags = ifm->ifm_flags;
 #if defined(__bsdi__)
@@ -379,13 +356,11 @@ ifm_read (struct if_msghdr *ifm)
 #endif /* __bsdi__ */
       if_get_metric (ifp);
 
-      /* Fetch hardware address. */
-      if (sdl->sdl_family != AF_LINK)
-	{
-	  zlog_warn ("sockaddr_dl->sdl_family is not AF_LINK");
-	  return -1;
-	}
-      /* XXX sockaddr_dl can be larger than base structure */
+      /* 
+       * XXX sockaddr_dl contents can be larger than the structure
+       * definition, so the user of the stored structure must be
+       * careful not to read off the end.
+       */
       memcpy (&ifp->sdl, sdl, sizeof (struct sockaddr_dl));
 
       if_add_update (ifp);
@@ -438,7 +413,7 @@ ifam_read_mesg (struct ifa_msghdr *ifm,
 #define IFAMADDRGET(X,R) \
     if (ifm->ifam_addrs & (R)) \
       { \
-        int len = WRAPUP(pnt); \
+        int len = SAROUNDUP(pnt); \
         if (((X) != NULL) && af_check (((struct sockaddr *)pnt)->sa_family)) \
           memcpy ((caddr_t)(X), pnt, len); \
         pnt += len; \
@@ -446,7 +421,7 @@ ifam_read_mesg (struct ifa_msghdr *ifm,
 #define IFAMMASKGET(X,R) \
     if (ifm->ifam_addrs & (R)) \
       { \
-	int len = WRAPUP(pnt); \
+	int len = SAROUNDUP(pnt); \
         if ((X) != NULL) \
 	  memcpy ((caddr_t)(X), pnt, len); \
 	pnt += len; \
@@ -554,7 +529,7 @@ rtm_read_mesg (struct rt_msghdr *rtm,
 #define RTMADDRGET(X,R) \
     if (rtm->rtm_addrs & (R)) \
       { \
-	int len = WRAPUP (pnt); \
+	int len = SAROUNDUP (pnt); \
         if (((X) != NULL) && af_check (((struct sockaddr *)pnt)->sa_family)) \
 	  memcpy ((caddr_t)(X), pnt, len); \
 	pnt += len; \
@@ -562,7 +537,7 @@ rtm_read_mesg (struct rt_msghdr *rtm,
 #define RTMMASKGET(X,R) \
     if (rtm->rtm_addrs & (R)) \
       { \
-	int len = WRAPUP (pnt); \
+	int len = SAROUNDUP (pnt); \
         if ((X) != NULL) \
 	  memcpy ((caddr_t)(X), pnt, len); \
 	pnt += len; \