Fix the initial check for NULL arguments in rtfree (previously

it checked for rt == NULL after dereferencing the pointer).
We never check for those events elsewhere, so probably these checks
might go away here as well.

Slightly simplify (and document) the logic for memory allocation
in rt_setgate().

The rest is mostly style changes -- replace 0 with NULL where appropriate,
remove the macro SA() that was only used once, remove some useless
debugging code in rt_fixchange, explain some odd-looking casts.

1 files changed, 82 insertions, 126 deletions

diff --git a/sys/net/route.c b/sys/net/route.c
index b8f000d..a4623b1 100644
--- a/sys/net/route.c
+++ b/sys/net/route.c
@@ -47,8 +47,6 @@
 #include <netinet/in.h>
 #include <netinet/ip_mroute.h>
 
-#define	SA(p) ((struct sockaddr *)(p))
-
 static struct rtstat rtstat;
 struct radix_node_head *rt_tables[AF_MAX+1];
 
@@ -117,7 +115,7 @@ rtalloc1(struct sockaddr *dst, int report, u_long ignflags)
 	u_long nflags;
 	int err = 0, msgtype = RTM_MISS;
 
-	newrt = 0;
+	newrt = NULL;
 	bzero(&info, sizeof(info));
 	/*
 	 * Look up the address in the table for that Address Family
@@ -141,8 +139,8 @@ rtalloc1(struct sockaddr *dst, int report, u_long ignflags)
 			 * If it requires that it be cloned, do so.
 			 * (This implies it wasn't a HOST route.)
 			 */
-			err = rtrequest(RTM_RESOLVE, dst, SA(0),
-					      SA(0), 0, &newrt);
+			err = rtrequest(RTM_RESOLVE, dst, NULL,
+					      NULL, 0, &newrt);
 			if (err) {
 				/*
 				 * If the cloning didn't succeed, maybe
@@ -210,13 +208,14 @@ rtalloc1(struct sockaddr *dst, int report, u_long ignflags)
 void
 rtfree(struct rtentry *rt)
 {
-	/*
-	 * find the tree for that address family
-	 */
-	struct radix_node_head *rnh = rt_tables[rt_key(rt)->sa_family];
+	struct radix_node_head *rnh;
 
-	if (rt == 0 || rnh == 0)
-		panic("rtfree");
+	/* XXX the NULL checks are probably useless */
+	if (rt == NULL)
+		panic("rtfree: NULL rt");
+	rnh = rt_tables[rt_key(rt)->sa_family];
+	if (rnh == NULL)
+		panic("rtfree: NULL rnh");
 
 	RT_LOCK_ASSERT(rt);
 
@@ -303,12 +302,12 @@ rtredirect(struct sockaddr *dst,
 {
 	struct rtentry *rt;
 	int error = 0;
-	short *stat = 0;
+	short *stat = NULL;
 	struct rt_addrinfo info;
 	struct ifaddr *ifa;
 
 	/* verify the gateway is directly reachable */
-	if ((ifa = ifa_ifwithnet(gateway)) == 0) {
+	if ((ifa = ifa_ifwithnet(gateway)) == NULL) {
 		error = ENETUNREACH;
 		goto out;
 	}
@@ -332,7 +331,7 @@ rtredirect(struct sockaddr *dst,
 	 * which use routing redirects generated by smart gateways
 	 * to dynamically build the routing tables.
 	 */
-	if (rt == 0 || (rt_mask(rt) && rt_mask(rt)->sa_len < 2))
+	if (rt == NULL || (rt_mask(rt) && rt_mask(rt)->sa_len < 2))
 		goto create;
 	/*
 	 * Don't listen to the redirect if it's
@@ -419,11 +418,10 @@ ifa_ifwithroute(int flags, struct sockaddr *dst, struct sockaddr *gateway)
 		 * as our clue to the interface.  Otherwise
 		 * we can use the local address.
 		 */
-		ifa = 0;
-		if (flags & RTF_HOST) {
+		ifa = NULL;
+		if (flags & RTF_HOST)
 			ifa = ifa_ifwithdstaddr(dst);
-		}
-		if (ifa == 0)
+		if (ifa == NULL)
 			ifa = ifa_ifwithaddr(gateway);
 	} else {
 		/*
@@ -433,28 +431,28 @@ ifa_ifwithroute(int flags, struct sockaddr *dst, struct sockaddr *gateway)
 		 */
 		ifa = ifa_ifwithdstaddr(gateway);
 	}
-	if (ifa == 0)
+	if (ifa == NULL)
 		ifa = ifa_ifwithnet(gateway);
-	if (ifa == 0) {
+	if (ifa == NULL) {
 		struct rtentry *rt = rtalloc1(gateway, 0, 0UL);
-		if (rt == 0)
-			return (0);
+		if (rt == NULL)
+			return (NULL);
 		RT_REMREF(rt);
 		RT_UNLOCK(rt);
-		if ((ifa = rt->rt_ifa) == 0)
-			return (0);
+		if ((ifa = rt->rt_ifa) == NULL)
+			return (NULL);
 	}
 	if (ifa->ifa_addr->sa_family != dst->sa_family) {
 		struct ifaddr *oifa = ifa;
 		ifa = ifaof_ifpforaddr(dst, ifa->ifa_ifp);
-		if (ifa == 0)
+		if (ifa == NULL)
 			ifa = oifa;
 	}
 	return (ifa);
 }
 
-static int rt_fixdelete(struct radix_node *, void *);
-static int rt_fixchange(struct radix_node *, void *);
+static walktree_f_t rt_fixdelete;
+static walktree_f_t rt_fixchange;
 
 struct rtfc_arg {
 	struct rtentry *rt0;
@@ -555,7 +553,7 @@ rtexpunge(struct rtentry *rt)
 	 * Find the correct routing tree to use for this Address Family
 	 */
 	rnh = rt_tables[rt_key(rt)->sa_family];
-	if (rnh == 0)
+	if (rnh == NULL)
 		return (EAFNOSUPPORT);
 
 	RADIX_NODE_HEAD_LOCK(rnh);
@@ -565,7 +563,7 @@ rtexpunge(struct rtentry *rt)
 	 * but when callers invoke us blindly it may not (sigh).
 	 */
 	rn = rnh->rnh_deladdr(rt_key(rt), rt_mask(rt), rnh);
-	if (rn == 0) {
+	if (rn == NULL) {
 		error = ESRCH;
 		goto bad;
 	}
@@ -592,7 +590,7 @@ rtexpunge(struct rtentry *rt)
 	if (rt->rt_gwroute) {
 		struct rtentry *gwrt = rt->rt_gwroute;
 		RTFREE(gwrt);
-		rt->rt_gwroute = 0;
+		rt->rt_gwroute = NULL;
 	}
 
 	/*
@@ -634,7 +632,7 @@ rtrequest1(int req, struct rt_addrinfo *info, struct rtentry **ret_nrt)
 	 * Find the correct routing tree to use for this Address Family
 	 */
 	rnh = rt_tables[dst->sa_family];
-	if (rnh == 0)
+	if (rnh == NULL)
 		return (EAFNOSUPPORT);
 	RADIX_NODE_HEAD_LOCK(rnh);
 	/*
@@ -642,7 +640,7 @@ rtrequest1(int req, struct rt_addrinfo *info, struct rtentry **ret_nrt)
 	 * a netmask in the tree, nor do we want to clone it.
 	 */
 	if (flags & RTF_HOST) {
-		netmask = 0;
+		netmask = NULL;
 		flags &= ~RTF_CLONING;
 	}
 	switch (req) {
@@ -652,7 +650,7 @@ rtrequest1(int req, struct rt_addrinfo *info, struct rtentry **ret_nrt)
 		 * Complain if it is not there and do no more processing.
 		 */
 		rn = rnh->rnh_deladdr(dst, netmask, rnh);
-		if (rn == 0)
+		if (rn == NULL)
 			senderr(ESRCH);
 		if (rn->rn_flags & (RNF_ACTIVE | RNF_ROOT))
 			panic ("rtrequest delete");
@@ -679,7 +677,7 @@ rtrequest1(int req, struct rt_addrinfo *info, struct rtentry **ret_nrt)
 		if (rt->rt_gwroute) {
 			struct rtentry *gwrt = rt->rt_gwroute;
 			RTFREE(gwrt);
-			rt->rt_gwroute = 0;
+			rt->rt_gwroute = NULL;
 		}
 
 		/*
@@ -707,7 +705,7 @@ rtrequest1(int req, struct rt_addrinfo *info, struct rtentry **ret_nrt)
 		break;
 
 	case RTM_RESOLVE:
-		if (ret_nrt == 0 || (rt = *ret_nrt) == 0)
+		if (ret_nrt == NULL || (rt = *ret_nrt) == NULL)
 			senderr(EINVAL);
 		ifa = rt->rt_ifa;
 		/* XXX locking? */
@@ -715,7 +713,7 @@ rtrequest1(int req, struct rt_addrinfo *info, struct rtentry **ret_nrt)
 		    ~(RTF_CLONING | RTF_STATIC);
 		flags |= RTF_WASCLONED;
 		gateway = rt->rt_gateway;
-		if ((netmask = rt->rt_genmask) == 0)
+		if ((netmask = rt->rt_genmask) == NULL)
 			flags |= RTF_HOST;
 		goto makeroute;
 
@@ -729,7 +727,7 @@ rtrequest1(int req, struct rt_addrinfo *info, struct rtentry **ret_nrt)
 
 	makeroute:
 		R_Zalloc(rt, struct rtentry *, sizeof(*rt));
-		if (rt == 0)
+		if (rt == NULL)
 			senderr(ENOBUFS);
 		RT_LOCK_INIT(rt);
 		rt->rt_flags = RTF_UP | flags;
@@ -768,7 +766,7 @@ rtrequest1(int req, struct rt_addrinfo *info, struct rtentry **ret_nrt)
 
 		/* XXX mtu manipulation will be done in rnh_addaddr -- itojun */
 		rn = rnh->rnh_addaddr(ndst, netmask, rnh, rt->rt_nodes);
-		if (rn == 0) {
+		if (rn == NULL) {
 			struct rtentry *rt2;
 			/*
 			 * Uh-oh, we already have one of these in the tree.
@@ -793,7 +791,7 @@ rtrequest1(int req, struct rt_addrinfo *info, struct rtentry **ret_nrt)
 		 * If it still failed to go into the tree,
 		 * then un-make it (this should be a function)
 		 */
-		if (rn == 0) {
+		if (rn == NULL) {
 			if (rt->rt_gwroute)
 				RTFREE(rt->rt_gwroute);
 			if (rt->rt_ifa)
@@ -804,7 +802,7 @@ rtrequest1(int req, struct rt_addrinfo *info, struct rtentry **ret_nrt)
 			senderr(EEXIST);
 		}
 
-		rt->rt_parent = 0;
+		rt->rt_parent = NULL;
 
 		/*
 		 * If we got here from RESOLVE, then we are cloning
@@ -844,7 +842,7 @@ rtrequest1(int req, struct rt_addrinfo *info, struct rtentry **ret_nrt)
 		 * hasn't been added to the tree yet.
 		 */
 		if (req == RTM_ADD &&
-		    !(rt->rt_flags & RTF_HOST) && rt_mask(rt) != 0) {
+		    !(rt->rt_flags & RTF_HOST) && rt_mask(rt) != NULL) {
 			struct rtfc_arg arg;
 			arg.rnh = rnh;
 			arg.rt0 = rt;
@@ -888,14 +886,14 @@ bad:
 static int
 rt_fixdelete(struct radix_node *rn, void *vp)
 {
+	/* The cast is safe because *rt starts with a struct radix_node. */
 	struct rtentry *rt = (struct rtentry *)rn;
 	struct rtentry *rt0 = vp;
 
 	if (rt->rt_parent == rt0 &&
 	    !(rt->rt_flags & (RTF_PINNED | RTF_CLONING))) {
-		return rtrequest(RTM_DELETE, rt_key(rt),
-				 (struct sockaddr *)0, rt_mask(rt),
-				 rt->rt_flags, (struct rtentry **)0);
+		return rtrequest(RTM_DELETE, rt_key(rt), NULL, rt_mask(rt),
+				 rt->rt_flags, NULL);
 	}
 	return 0;
 }
@@ -913,13 +911,11 @@ rt_fixdelete(struct radix_node *rn, void *vp)
  * routine just for adds.  I'm not sure why I thought it was necessary to do
  * changes this way.
  */
-#ifdef DEBUG
-static int rtfcdebug = 0;
-#endif
 
 static int
 rt_fixchange(struct radix_node *rn, void *vp)
 {
+	/* The cast is safe because *rt starts with a struct radix_node. */
 	struct rtentry *rt = (struct rtentry *)rn;
 	struct rtfc_arg *ap = vp;
 	struct rtentry *rt0 = ap->rt0;
@@ -927,28 +923,13 @@ rt_fixchange(struct radix_node *rn, void *vp)
 	u_char *xk1, *xm1, *xk2, *xmp;
 	int i, len, mlen;
 
-#ifdef DEBUG
-	if (rtfcdebug)
-		printf("rt_fixchange: rt %p, rt0 %p\n", rt, rt0);
-#endif
-
+	/* make sure we have a parent, and route is not pinned or cloning */
 	if (!rt->rt_parent ||
-	    (rt->rt_flags & (RTF_PINNED | RTF_CLONING))) {
-#ifdef DEBUG
-		if(rtfcdebug) printf("no parent, pinned or cloning\n");
-#endif
+	    (rt->rt_flags & (RTF_PINNED | RTF_CLONING)))
 		return 0;
-	}
-
-	if (rt->rt_parent == rt0) {
-#ifdef DEBUG
-		if(rtfcdebug) printf("parent match\n");
-#endif
-		return rtrequest(RTM_DELETE, rt_key(rt),
-				 (struct sockaddr *)0, rt_mask(rt),
-				 rt->rt_flags, (struct rtentry **)0);
-	}
 
+	if (rt->rt_parent == rt0)	/* parent match */
+		goto delete_rt;
 	/*
 	 * There probably is a function somewhere which does this...
 	 * if not, there should be.
@@ -962,43 +943,23 @@ rt_fixchange(struct radix_node *rn, void *vp)
 	/* avoid applying a less specific route */
 	xmp = (u_char *)rt_mask(rt->rt_parent);
 	mlen = rt_key(rt->rt_parent)->sa_len;
-	if (mlen > rt_key(rt0)->sa_len) {
-#ifdef DEBUG
-		if (rtfcdebug)
-			printf("rt_fixchange: inserting a less "
-			       "specific route\n");
-#endif
+	if (mlen > rt_key(rt0)->sa_len)		/* less specific route */
 		return 0;
-	}
-	for (i = rnh->rnh_treetop->rn_offset; i < mlen; i++) {
-		if ((xmp[i] & ~(xmp[i] ^ xm1[i])) != xmp[i]) {
-#ifdef DEBUG
-			if (rtfcdebug)
-				printf("rt_fixchange: inserting a less "
-				       "specific route\n");
-#endif
-			return 0;
-		}
-	}
+	for (i = rnh->rnh_treetop->rn_offset; i < mlen; i++)
+		if ((xmp[i] & ~(xmp[i] ^ xm1[i])) != xmp[i])
+			return 0;	/* less specific route */
 
-	for (i = rnh->rnh_treetop->rn_offset; i < len; i++) {
-		if ((xk2[i] & xm1[i]) != xk1[i]) {
-#ifdef DEBUG
-			if(rtfcdebug) printf("no match\n");
-#endif
-			return 0;
-		}
-	}
+	for (i = rnh->rnh_treetop->rn_offset; i < len; i++)
+		if ((xk2[i] & xm1[i]) != xk1[i])
+			return 0;	/* no match */
 
 	/*
 	 * OK, this node is a clone, and matches the node currently being
 	 * changed/added under the node's mask.  So, get rid of it.
 	 */
-#ifdef DEBUG
-	if(rtfcdebug) printf("deleting\n");
-#endif
-	return rtrequest(RTM_DELETE, rt_key(rt), (struct sockaddr *)0,
-			 rt_mask(rt), rt->rt_flags, (struct rtentry **)0);
+delete_rt:
+	return rtrequest(RTM_DELETE, rt_key(rt), NULL,
+			 rt_mask(rt), rt->rt_flags, NULL);
 }
 
 int
@@ -1006,7 +967,6 @@ rt_setgate(struct rtentry *rt, struct sockaddr *dst, struct sockaddr *gate)
 {
 	/* XXX dst may be overwritten, can we move this to below */
 	struct radix_node_head *rnh = rt_tables[dst->sa_family];
-	caddr_t new, old;
 	int dlen = SA_SIZE(dst), glen = SA_SIZE(gate);
 
 	RT_LOCK_ASSERT(rt);
@@ -1030,39 +990,35 @@ rt_setgate(struct rtentry *rt, struct sockaddr *dst, struct sockaddr *gate)
 	}
 
 	/*
-	 * Both dst and gateway are stored in the same malloc'd chunk
-	 * (If I ever get my hands on....)
-	 * if we need to malloc a new chunk, then keep the old one around
-	 * till we don't need it any more.
+	 * Prepare to store the gateway in rt->rt_gateway.
+	 * Both dst and gateway are stored one after the other in the same
+	 * malloc'd chunk. If we have room, we can reuse the old buffer,
+	 * rt_gateway already points to the right place.
+	 * Otherwise, malloc a new block and update the 'dst' address.
 	 */
-	if (rt->rt_gateway == 0 || glen > SA_SIZE(rt->rt_gateway)) {
-		old = (caddr_t)rt_key(rt);
+	if (rt->rt_gateway == NULL || glen > SA_SIZE(rt->rt_gateway)) {
+		caddr_t new;
+
 		R_Malloc(new, caddr_t, dlen + glen);
-		if (new == 0)
+		if (new == NULL)
 			return ENOBUFS;
-		rt_key(rt) = new;
-	} else {
 		/*
-		 * otherwise just overwrite the old one
+		 * XXX note, we copy from *dst and not *rt_key(rt) because
+		 * rt_setgate() can be called to initialize a newly
+		 * allocated route entry, in which case rt_key(rt) == NULL
+		 * (and also rt->rt_gateway == NULL).
+		 * Free()/free() handle a NULL argument just fine.
 		 */
-		new = (caddr_t)rt_key(rt);
-		old = 0;
+		bcopy(dst, new, dlen);
+		Free(rt_key(rt));	/* free old block, if any */
+		rt_key(rt) = new;
+		rt->rt_gateway = (struct sockaddr *)(new + dlen);
 	}
 
 	/*
-	 * copy the new gateway value into the memory chunk
+	 * Copy the new gateway value into the memory chunk.
 	 */
-	bcopy(gate, (rt->rt_gateway = (struct sockaddr *)(new + dlen)), glen);
-
-	/*
-	 * if we are replacing the chunk (or it's new) we need to
-	 * replace the dst as well
-	 */
-	if (old) {
-		bcopy(dst, new, dlen);
-		Free(old);
-		old = 0;
-	}
+	bcopy(gate, rt->rt_gateway, glen);
 
 	/*
 	 * If there is already a gwroute, it's now almost definitly wrong
@@ -1148,8 +1104,8 @@ rtinit(struct ifaddr *ifa, int cmd, int flags)
 {
 	struct sockaddr *dst;
 	struct sockaddr *netmask;
-	struct mbuf *m = 0;
-	struct rtentry *rt = 0;
+	struct mbuf *m = NULL;
+	struct rtentry *rt = NULL;
 	struct rt_addrinfo info;
 	int error;
 
@@ -1193,7 +1149,7 @@ rtinit(struct ifaddr *ifa, int cmd, int flags)
 		error = ((rn = rnh->rnh_lookup(dst, netmask, rnh)) == NULL ||
 		    (rn->rn_flags & RNF_ROOT) ||
 		    ((struct rtentry *)rn)->rt_ifa != ifa ||
-		    !sa_equal(SA(rn->rn_key), dst));
+		    !sa_equal((struct sockaddr *)rn->rn_key, dst));
 		RADIX_NODE_HEAD_UNLOCK(rnh);
 		if (error) {
 bad:
@@ -1278,7 +1234,7 @@ rt_check(struct rtentry **lrt, struct rtentry **lrt0, struct sockaddr *dst)
 		}
 		/* XXX BSD/OS checks dst->sa_family != AF_NS */
 		if (rt->rt_flags & RTF_GATEWAY) {
-			if (rt->rt_gwroute == 0)
+			if (rt->rt_gwroute == NULL)
 				goto lookup;
 			rt = rt->rt_gwroute;
 			RT_LOCK(rt);		/* NB: gwroute */
@@ -1290,7 +1246,7 @@ rt_check(struct rtentry **lrt, struct rtentry **lrt0, struct sockaddr *dst)
 				rt = rtalloc1(rt->rt_gateway, 1, 0UL);
 				RT_LOCK(rt0);
 				rt0->rt_gwroute = rt;
-				if (rt == 0) {
+				if (rt == NULL) {
 					RT_UNLOCK(rt0);
 					senderr(EHOSTUNREACH);
 				}