Hi! The attached patch fixes the logic of rtrequest1() when handling RTM_DELETE requests and the caller asked for a copy of the removed entry.
Reviewers are highly welcome. This makes the code more natural, similar to the RTM_ADD and RTM_RESOLVE cases (wrt to handling the returned rtentry's refcnt), and reduces some code bloat in applications. It also makes it safe to turn the RTF_UP bit before rt_fixdelete() walk -- that was the start of my looking at this part of code. Julian, keeping RTF_UP during the rt_fixdelete() walk was necessary because we remove any cloned routes. When the last cloned route is getting removed, it calls RTFREE(rt->rt_parent), and is this was a last reference, it would blow our route away. Does this answer your question in route.c,v 1.41 from 05-Mar-97? You were right that this is handled better by holding a reference on the route (by incrementing rt_refcnt). Cheers, -- Ruslan Ermilov Sysadmin and DBA, [EMAIL PROTECTED] Sunbay Software AG, [EMAIL PROTECTED] FreeBSD committer, +380.652.512.251 Simferopol, Ukraine http://www.FreeBSD.org The Power To Serve http://www.oracle.com Enabling The Information Age
Index: net/route.c
===================================================================
RCS file: /home/ncvs/src/sys/net/route.c,v
retrieving revision 1.72
diff -u -p -r1.72 route.c
--- net/route.c 18 Dec 2002 11:45:27 -0000 1.72
+++ net/route.c 22 Dec 2002 19:07:24 -0000
@@ -557,6 +557,8 @@ rtrequest1(req, info, ret_nrt)
if (rn->rn_flags & (RNF_ACTIVE | RNF_ROOT))
panic ("rtrequest delete");
rt = (struct rtentry *)rn;
+ rt->rt_refcnt++;
+ rt->rt_flags &= ~RTF_UP;
/*
* Now search what's left of the subtree for any cloned
@@ -580,15 +582,6 @@ rtrequest1(req, info, ret_nrt)
}
/*
- * NB: RTF_UP must be set during the search above,
- * because we might delete the last ref, causing
- * rt to get freed prematurely.
- * eh? then why not just add a reference?
- * I'm not sure how RTF_UP helps matters. (JRE)
- */
- rt->rt_flags &= ~RTF_UP;
-
- /*
* give the protocol a chance to keep things in sync.
*/
if ((ifa = rt->rt_ifa) && ifa->ifa_rtrequest)
@@ -607,10 +600,8 @@ rtrequest1(req, info, ret_nrt)
*/
if (ret_nrt)
*ret_nrt = rt;
- else if (rt->rt_refcnt <= 0) {
- rt->rt_refcnt++; /* make a 1->0 transition */
- rtfree(rt);
- }
+ else
+ RTFREE(rt);
break;
case RTM_RESOLVE:
Index: net/rtsock.c
===================================================================
RCS file: /home/ncvs/src/sys/net/rtsock.c,v
retrieving revision 1.80
diff -u -p -r1.80 rtsock.c
--- net/rtsock.c 18 Dec 2002 11:45:27 -0000 1.80
+++ net/rtsock.c 22 Dec 2002 19:07:25 -0000
@@ -356,8 +356,7 @@ route_output(m, so)
case RTM_DELETE:
error = rtrequest1(RTM_DELETE, &info, &saved_nrt);
if (error == 0) {
- if ((rt = saved_nrt))
- rt->rt_refcnt++;
+ rt = saved_nrt;
goto report;
}
break;
Index: netinet6/in6.c
===================================================================
RCS file: /home/ncvs/src/sys/netinet6/in6.c,v
retrieving revision 1.22
diff -u -p -r1.22 in6.c
--- netinet6/in6.c 18 Dec 2002 11:46:59 -0000 1.22
+++ netinet6/in6.c 22 Dec 2002 19:07:26 -0000
@@ -197,11 +197,7 @@ in6_ifloop_request(int cmd, struct ifadd
if (nrt) {
rt_newaddrmsg(cmd, ifa, e, nrt);
if (cmd == RTM_DELETE) {
- if (nrt->rt_refcnt <= 0) {
- /* XXX: we should free the entry ourselves. */
- nrt->rt_refcnt++;
- rtfree(nrt);
- }
+ RTFREE(nrt);
} else {
/* the cmd must be RTM_ADD here */
nrt->rt_refcnt--;
Index: netinet6/nd6_rtr.c
===================================================================
RCS file: /home/ncvs/src/sys/netinet6/nd6_rtr.c,v
retrieving revision 1.11
diff -u -p -r1.11 nd6_rtr.c
--- netinet6/nd6_rtr.c 19 Apr 2002 04:46:23 -0000 1.11
+++ netinet6/nd6_rtr.c 22 Dec 2002 19:07:26 -0000
@@ -574,14 +574,7 @@ defrouter_delreq(dr, dofree)
RTF_GATEWAY, &oldrt);
if (oldrt) {
nd6_rtmsg(RTM_DELETE, oldrt);
- if (oldrt->rt_refcnt <= 0) {
- /*
- * XXX: borrowed from the RTM_DELETE case of
- * rtrequest().
- */
- oldrt->rt_refcnt++;
- rtfree(oldrt);
- }
+ RTFREE(oldrt);
}
if (dofree) /* XXX: necessary? */
@@ -1583,13 +1576,8 @@ nd6_prefix_offlink(pr)
error));
}
- if (rt != NULL) {
- if (rt->rt_refcnt <= 0) {
- /* XXX: we should free the entry ourselves. */
- rt->rt_refcnt++;
- rtfree(rt);
- }
- }
+ if (rt != NULL)
+ RTFREE(rt);
return(error);
}
msg07913/pgp00000.pgp
Description: PGP signature
