Subject: Re: multiple copies of arp_lock_try()
To: Atsushi Onoe <onoe@sm.sony.co.jp>
From: Jun-ichiro itojun Hagino <itojun@iijlab.net>
List: tech-net
Date: 06/24/2002 15:37:52
>> 	another question - why do we need if_ieee1394arp.c?  i took a diff
>> 	between if_arp.c and i saw almost no difference...
>Hmm.  I can't recall the reason but it seems simply because 1394ARP is
>defined differently from normal ARP.  It doesn't have target hardware
>address, and the length of hardware address is defined as 16 rather
>than 8 (IEEE1394_ADDR_LEN).
>But as you see, we designed 1394 interface with address length 16 which is
>the same format as 1394ARP.  The difference in code is minimal as a result.
>So we may merge it into if_arp.c, though my knowledge about 1394 is almost
>gone...

	if the only difference is the lack of target hardware address, does the
	following diff look sane?  i really need an environment to test it...

itojun


Index: netinet/if_ieee1394arp.c - remove
Index: netinet/if_ieee1394arp.h - remove

Index: conf/files
===================================================================
RCS file: /cvsroot/syssrc/sys/conf/files,v
retrieving revision 1.535
diff -u -r1.535 files
--- conf/files	2002/06/18 00:33:57	1.535
+++ conf/files	2002/06/24 06:34:13
@@ -1156,7 +1156,6 @@
 file	netccitt/pk_usrreq.c		ccitt
 file	netinet/fil.c			ipfilter
 file	netinet/if_arp.c		arp | netatalk		needs-flag
-file	netinet/if_ieee1394arp.c	arp & ieee1394 & inet
 file	netinet/if_atm.c		atm
 file	netinet/igmp.c			inet
 file	netinet/in.c			inet
Index: net/if_arp.h
===================================================================
RCS file: /cvsroot/syssrc/sys/net/if_arp.h,v
retrieving revision 1.22
diff -u -r1.22 if_arp.h
--- net/if_arp.h	2001/06/12 15:17:27	1.22
+++ net/if_arp.h	2002/06/24 06:34:13
@@ -76,9 +76,14 @@
 	u_int8_t  ar_tpa[];	/* target protocol address */
 #endif
 #define ar_sha(ap) (((caddr_t)((ap)+1))+0)
-#define ar_spa(ap) (((caddr_t)((ap)+1))+  (ap)->ar_hln)
-#define ar_tha(ap) (((caddr_t)((ap)+1))+  (ap)->ar_hln+(ap)->ar_pln)
-#define ar_tpa(ap) (((caddr_t)((ap)+1))+2*(ap)->ar_hln+(ap)->ar_pln)
+#define ar_spa(ap) (((caddr_t)((ap)+1))+(ap)->ar_hln)
+#define ar_tha(ap) \
+	(ntohs((ap)->ar_hrd) == ARPHRD_IEEE1394 \
+		? NULL : (((caddr_t)((ap)+1))+(ap)->ar_hln+(ap)->ar_pln))
+#define ar_tpa(ap) \
+	(ntohs((ap)->ar_hrd) == ARPHRD_IEEE1394 \
+		? (((caddr_t)((ap)+1))+(ap)->ar_hln+(ap)->ar_pln) \
+		: (((caddr_t)((ap)+1))+(ap)->ar_hln+(ap)->ar_pln+(ap)->ar_hln))
 } __attribute__((__packed__));
 
 
Index: net/if_ieee1394subr.c
===================================================================
RCS file: /cvsroot/syssrc/sys/net/if_ieee1394subr.c,v
retrieving revision 1.15
diff -u -r1.15 if_ieee1394subr.c
--- net/if_ieee1394subr.c	2002/05/16 09:08:33	1.15
+++ net/if_ieee1394subr.c	2002/06/24 06:34:13
@@ -66,7 +66,7 @@
 #ifdef INET
 #include <netinet/in.h>
 #include <netinet/in_var.h>
-#include <netinet/if_ieee1394arp.h>
+#include <netinet/if_inarp.h>
 #endif /* INET */
 #ifdef INET6
 #include <netinet/in.h>
@@ -93,9 +93,6 @@
 	struct rtentry *rt;
 	struct mbuf *mcopy = NULL;
 	struct ieee1394_hwaddr hwdst, *myaddr;
-#ifdef INET
-	struct ieee1394_arphdr *ah;
-#endif /* INET */
 	ALTQ_DECL(struct altq_pktattr pktattr;)
 
 	if ((ifp->if_flags & (IFF_UP|IFF_RUNNING)) != (IFF_UP|IFF_RUNNING))
@@ -146,7 +143,7 @@
 	case AF_INET:
 		if (m0->m_flags & (M_BCAST | M_MCAST))
 			memcpy(&hwdst, ifp->if_broadcastaddr, sizeof(hwdst));
-		else if (!ieee1394arpresolve(ifp, rt, m0, dst, &hwdst))
+		else if (!arpresolve(ifp, rt, m0, dst, (u_char *)&hwdst))
 			return 0;	/* if not yet resolved */
 		/* if broadcasting on a simplex interface, loopback a copy */
 		if ((m0->m_flags & M_BCAST) && (ifp->if_flags & IFF_SIMPLEX))
@@ -154,7 +151,6 @@
 		etype = htons(ETHERTYPE_IP);
 		break;
 	case AF_ARP:
-		ah = mtod(m0, struct ieee1394_arphdr *);
 		memcpy(&hwdst, ifp->if_broadcastaddr, sizeof(hwdst));
 		etype = htons(ETHERTYPE_ARP);
 		break;
@@ -388,7 +384,8 @@
 		break;
 
 	case ETHERTYPE_ARP:
-		in_ieee1394arpinput(m);
+		schednetisr(NETISR_ARP);
+		inq = &arpintrq;
 		return;
 #endif /* INET */
 
@@ -703,7 +700,7 @@
 			if ((error = fw_init(ifp)) != 0)
 #endif
 				break;
-			ieee1394arp_ifinit(ifp, ifa);
+			arp_ifinit(ifp, ifa);
 			break;
 #endif /* INET */
 		default:
Index: netinet/if_arp.c
===================================================================
RCS file: /cvsroot/syssrc/sys/netinet/if_arp.c,v
retrieving revision 1.81
diff -u -r1.81 if_arp.c
--- netinet/if_arp.c	2002/06/09 16:33:37	1.81
+++ netinet/if_arp.c	2002/06/24 06:34:14
@@ -110,7 +110,6 @@
 #include <net/if_types.h>
 #include <net/route.h>
 
-
 #include <netinet/in.h>
 #include <netinet/in_systm.h>
 #include <netinet/in_var.h>
@@ -247,9 +246,8 @@
  * protection of the ARP_LOCK; if arp_drain() or arptimer is called
  * while the arp table is locked, we punt and try again later.
  */
-
-int	arp_locked;
 
+static int	arp_locked;
 static __inline int arp_lock_try __P((int));
 static __inline void arp_unlock __P((void));
 
@@ -1014,14 +1012,18 @@
 	arpstat.as_rcvrequest++;
 	if (in_hosteq(itaddr, myaddr)) {
 		/* I am the target */
-		bcopy((caddr_t)ar_sha(ah), (caddr_t)ar_tha(ah), ah->ar_hln);
+		if (ar_tha(ah))
+			bcopy((caddr_t)ar_sha(ah), (caddr_t)ar_tha(ah),
+			    ah->ar_hln);
 		bcopy(LLADDR(ifp->if_sadl), (caddr_t)ar_sha(ah), ah->ar_hln);
 	} else {
 		la = arplookup(m, &itaddr, 0, SIN_PROXY);
 		if (la == 0)
 			goto out;
 		rt = la->la_rt;
-		bcopy((caddr_t)ar_sha(ah), (caddr_t)ar_tha(ah), ah->ar_hln);
+		if (ar_tha(ah))
+			bcopy((caddr_t)ar_sha(ah), (caddr_t)ar_tha(ah),
+			    ah->ar_hln);
 		sdl = SDL(rt->rt_gateway);
 		bcopy(LLADDR(sdl), (caddr_t)ar_sha(ah), ah->ar_hln);
 	}
@@ -1193,6 +1195,11 @@
 
 	ah = mtod(m, struct arphdr *);
 	op = ntohs(ah->ar_op);
+
+	/* ARP without target hardware address (IEEE1394) is not supported */
+	if (!ar_tha(ah))
+		goto out;
+
 	switch (op) {
 	case ARPOP_REQUEST:
 	case ARPOP_REPLY:	/* per RFC */
@@ -1410,4 +1417,3 @@
 }
 #endif
 #endif /* INET */
-