On 19/08/15(Wed) 13:06, Alexander Bluhm wrote: > Hi, > > I have converted all calls to rtrequest1() into a common pattern. > In the man page I tried to clarify the usage of the returned route. > > ok?
Ok. Your manpage change might lead to confusions because the code is tricky. If you pass a rt pointer to rtrequest1(9) in some cases th e reference counter of the route is incremented (RTM_ADD & RTM_RESOLVE) and in other not (RTM_DELETE). But my next change is to make this behavior consistent, so put it in I'll fix the code :) > > Index: sys/net/route.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.c,v > retrieving revision 1.221 > diff -u -p -r1.221 route.c > --- sys/net/route.c 18 Aug 2015 08:56:16 -0000 1.221 > +++ sys/net/route.c 19 Aug 2015 10:58:52 -0000 > @@ -301,9 +301,11 @@ struct rtentry * > rtalloc(struct sockaddr *dst, int flags, unsigned int tableid) > { > struct rtentry *rt; > - struct rtentry *newrt = 0; > + struct rtentry *newrt = NULL; > struct rt_addrinfo info; > - int s = splsoftnet(), err = 0, msgtype = RTM_MISS; > + int s, error = 0, msgtype = RTM_MISS; > + > + s = splsoftnet(); > > bzero(&info, sizeof(info)); > info.rti_info[RTAX_DST] = dst; > @@ -312,14 +314,15 @@ rtalloc(struct sockaddr *dst, int flags, > if (rt != NULL) { > newrt = rt; > if ((rt->rt_flags & RTF_CLONING) && ISSET(flags, RT_RESOLVE)) { > - err = rtrequest1(RTM_RESOLVE, &info, RTP_DEFAULT, > + error = rtrequest1(RTM_RESOLVE, &info, RTP_DEFAULT, > &newrt, tableid); > - if (err) { > + if (error) { > newrt = rt; > rt->rt_refcnt++; > goto miss; > } > - if ((rt = newrt) && (rt->rt_flags & RTF_XRESOLVE)) { > + rt = newrt; > + if (rt->rt_flags & RTF_XRESOLVE) { > msgtype = RTM_RESOLVE; > goto miss; > } > @@ -333,7 +336,7 @@ miss: > if (ISSET(flags, RT_REPORT)) { > bzero((caddr_t)&info, sizeof(info)); > info.rti_info[RTAX_DST] = dst; > - rt_missmsg(msgtype, &info, 0, NULL, err, tableid); > + rt_missmsg(msgtype, &info, 0, NULL, error, tableid); > } > } > splx(s); > @@ -510,7 +513,7 @@ create: > rt = NULL; > error = rtrequest1(RTM_ADD, &info, RTP_DEFAULT, &rt, > rdomain); > - if (rt != NULL) > + if (error == 0) > flags = rt->rt_flags; > stat = &rtstat.rts_dynamic; > } else { > @@ -1142,7 +1145,7 @@ int > rt_ifa_add(struct ifaddr *ifa, int flags, struct sockaddr *dst) > { > struct ifnet *ifp = ifa->ifa_ifp; > - struct rtentry *rt = NULL; > + struct rtentry *rt; > struct sockaddr_rtlabel sa_rl; > struct rt_addrinfo info; > u_short rtableid = ifp->if_rdomain; > @@ -1175,7 +1178,7 @@ rt_ifa_add(struct ifaddr *ifa, int flags > prio = RTP_LOCAL; > > error = rtrequest1(RTM_ADD, &info, prio, &rt, rtableid); > - if (error == 0 && rt != NULL) { > + if (error == 0) { > if (rt->rt_ifa != ifa) { > printf("%s: wrong ifa (%p) was (%p)\n", __func__, > ifa, rt->rt_ifa); > @@ -1206,7 +1209,7 @@ int > rt_ifa_del(struct ifaddr *ifa, int flags, struct sockaddr *dst) > { > struct ifnet *ifp = ifa->ifa_ifp; > - struct rtentry *rt, *nrt = NULL; > + struct rtentry *rt; > struct mbuf *m = NULL; > struct sockaddr *deldst; > struct rt_addrinfo info; > @@ -1257,11 +1260,11 @@ rt_ifa_del(struct ifaddr *ifa, int flags > if (flags & (RTF_LOCAL|RTF_BROADCAST)) > prio = RTP_LOCAL; > > - error = rtrequest1(RTM_DELETE, &info, prio, &nrt, rtableid); > - if (error == 0 && (rt = nrt) != NULL) { > - rt_sendmsg(nrt, RTM_DELETE, rtableid); > + error = rtrequest1(RTM_DELETE, &info, prio, &rt, rtableid); > + if (error == 0) { > + rt_sendmsg(rt, RTM_DELETE, rtableid); > if (flags & RTF_LOCAL) > - rt_sendaddrmsg(nrt, RTM_DELADDR); > + rt_sendaddrmsg(rt, RTM_DELADDR); > if (rt->rt_refcnt <= 0) { > rt->rt_refcnt++; > rtfree(rt); > Index: sys/net/rtsock.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/net/rtsock.c,v > retrieving revision 1.167 > diff -u -p -r1.167 rtsock.c > --- sys/net/rtsock.c 17 Aug 2015 09:46:26 -0000 1.167 > +++ sys/net/rtsock.c 19 Aug 2015 10:58:52 -0000 > @@ -595,7 +595,7 @@ route_output(struct mbuf *m, ...) > } > error = rtrequest1(rtm->rtm_type, &info, prio, &saved_nrt, > tableid); > - if (error == 0 && saved_nrt) { > + if (error == 0) { > rt_setmetrics(rtm->rtm_inits, &rtm->rtm_rmx, > &saved_nrt->rt_rmx); > /* write back the priority the kernel used */ > Index: sys/netinet/in_pcb.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in_pcb.c,v > retrieving revision 1.172 > diff -u -p -r1.172 in_pcb.c > --- sys/netinet/in_pcb.c 19 Jul 2015 02:35:35 -0000 1.172 > +++ sys/netinet/in_pcb.c 19 Aug 2015 11:00:17 -0000 > @@ -668,7 +668,7 @@ in_losing(struct inpcb *inp) > inp->inp_rtableid); > if (rt->rt_flags & RTF_DYNAMIC) > (void)rtrequest1(RTM_DELETE, &info, rt->rt_priority, > - (struct rtentry **)0, inp->inp_rtableid); > + NULL, inp->inp_rtableid); > /* > * A new route can be allocated > * the next time output is attempted. > Index: sys/netinet6/in6.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/in6.c,v > retrieving revision 1.163 > diff -u -p -r1.163 in6.c > --- sys/netinet6/in6.c 18 Aug 2015 08:48:36 -0000 1.163 > +++ sys/netinet6/in6.c 19 Aug 2015 10:58:52 -0000 > @@ -924,8 +924,8 @@ in6_update_ifa(struct ifnet *ifp, struct > info.rti_info[RTAX_NETMASK] = sin6tosa(&mltmask); > info.rti_info[RTAX_IFA] = sin6tosa(&ia6->ia_addr); > info.rti_flags = RTF_UP | RTF_CLONING; > - error = rtrequest1(RTM_ADD, &info, RTP_CONNECTED, > - NULL, ifp->if_rdomain); > + error = rtrequest1(RTM_ADD, &info, RTP_CONNECTED, NULL, > + ifp->if_rdomain); > if (error) > goto cleanup; > } else { > Index: sys/netinet6/nd6.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/nd6.c,v > retrieving revision 1.144 > diff -u -p -r1.144 nd6.c > --- sys/netinet6/nd6.c 17 Aug 2015 09:58:10 -0000 1.144 > +++ sys/netinet6/nd6.c 19 Aug 2015 10:58:52 -0000 > @@ -646,7 +646,7 @@ nd6_lookup(struct in6_addr *addr6, int c > if (!rt) { > if (create && ifp) { > struct rt_addrinfo info; > - int e; > + int error; > > /* > * If no route is available and create is set, > @@ -671,18 +671,9 @@ nd6_lookup(struct in6_addr *addr6, int c > info.rti_info[RTAX_DST] = sin6tosa(&sin6); > info.rti_info[RTAX_GATEWAY] = > (struct sockaddr *)ifp->if_sadl; > - if ((e = rtrequest1(RTM_ADD, &info, RTP_CONNECTED, > - &rt, rtableid)) != 0) { > -#if 0 > - char ip[INET6_ADDRSTRLEN]; > - log(LOG_ERR, "%s: failed to add route for a " > - "neighbor(%s), errno=%d\n", __func__, > - inet_ntop(AF_INET6, addr6, ip, sizeof(ip)), > - e); > -#endif > - return (NULL); > - } > - if (rt == NULL) > + error = rtrequest1(RTM_ADD, &info, RTP_CONNECTED, &rt, > + rtableid); > + if (error) > return (NULL); > if (rt->rt_llinfo) { > struct llinfo_nd6 *ln = > Index: sys/netinet6/nd6_rtr.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/nd6_rtr.c,v > retrieving revision 1.115 > diff -u -p -r1.115 nd6_rtr.c > --- sys/netinet6/nd6_rtr.c 18 Aug 2015 08:52:25 -0000 1.115 > +++ sys/netinet6/nd6_rtr.c 19 Aug 2015 10:58:52 -0000 > @@ -602,7 +602,7 @@ defrouter_addreq(struct nd_defrouter *ne > { > struct rt_addrinfo info; > struct sockaddr_in6 def, mask, gate; > - struct rtentry *rt = NULL; > + struct rtentry *rt; > int s; > int error; > > @@ -625,12 +625,11 @@ defrouter_addreq(struct nd_defrouter *ne > s = splsoftnet(); > error = rtrequest1(RTM_ADD, &info, RTP_DEFAULT, &rt, > new->ifp->if_rdomain); > - if (rt) { > + if (error == 0) { > rt_sendmsg(rt, RTM_ADD, new->ifp->if_rdomain); > rtfree(rt); > - } > - if (error == 0) > new->installed = 1; > + } > splx(s); > return; > } > @@ -705,7 +704,8 @@ defrouter_delreq(struct nd_defrouter *dr > { > struct rt_addrinfo info; > struct sockaddr_in6 def, mask, gw; > - struct rtentry *oldrt = NULL; > + struct rtentry *rt; > + int error; > > #ifdef DIAGNOSTIC > if (!dr) > @@ -728,17 +728,17 @@ defrouter_delreq(struct nd_defrouter *dr > info.rti_info[RTAX_GATEWAY] = sin6tosa(&gw); > info.rti_info[RTAX_NETMASK] = sin6tosa(&mask); > > - rtrequest1(RTM_DELETE, &info, RTP_DEFAULT, &oldrt, > + error = rtrequest1(RTM_DELETE, &info, RTP_DEFAULT, &rt, > dr->ifp->if_rdomain); > - if (oldrt) { > - rt_sendmsg(oldrt, RTM_DELETE, dr->ifp->if_rdomain); > - if (oldrt->rt_refcnt <= 0) { > + if (error == 0) { > + rt_sendmsg(rt, RTM_DELETE, dr->ifp->if_rdomain); > + if (rt->rt_refcnt <= 0) { > /* > * XXX: borrowed from the RTM_DELETE case of > * rtrequest1(). > */ > - oldrt->rt_refcnt++; > - rtfree(oldrt); > + rt->rt_refcnt++; > + rtfree(rt); > } > } > > @@ -1778,10 +1778,10 @@ nd6_prefix_onlink(struct nd_prefix *pr) > struct ifnet *ifp = pr->ndpr_ifp; > struct sockaddr_in6 mask6; > struct nd_prefix *opr; > - u_long rtflags; > - int error = 0; > - struct rtentry *rt = NULL; > + struct rtentry *rt; > char addr[INET6_ADDRSTRLEN]; > + u_long rtflags; > + int error; > > /* sanity check */ > if ((pr->ndpr_stateflags & NDPRF_ONLINK) != 0) > @@ -1856,7 +1856,7 @@ nd6_prefix_onlink(struct nd_prefix *pr) > info.rti_info[RTAX_NETMASK] = sin6tosa(&mask6); > > error = rtrequest1(RTM_ADD, &info, RTP_CONNECTED, &rt, ifp->if_rdomain); > - if (error == 0 && rt != NULL) { > + if (error == 0) { > pr->ndpr_stateflags |= NDPRF_ONLINK; > rt_sendmsg(rt, RTM_ADD, ifp->if_rdomain); > rtfree(rt); > @@ -1869,12 +1869,12 @@ int > nd6_prefix_offlink(struct nd_prefix *pr) > { > struct rt_addrinfo info; > - int error = 0; > struct ifnet *ifp = pr->ndpr_ifp; > struct nd_prefix *opr; > struct sockaddr_in6 sa6, mask6; > - struct rtentry *rt = NULL; > + struct rtentry *rt; > char addr[INET6_ADDRSTRLEN]; > + int error; > > /* sanity check */ > if ((pr->ndpr_stateflags & NDPRF_ONLINK) == 0) { > @@ -1898,14 +1898,14 @@ nd6_prefix_offlink(struct nd_prefix *pr) > bzero(&info, sizeof(info)); > info.rti_info[RTAX_DST] = sin6tosa(&sa6); > info.rti_info[RTAX_NETMASK] = sin6tosa(&mask6); > + > error = rtrequest1(RTM_DELETE, &info, RTP_CONNECTED, &rt, > ifp->if_rdomain); > if (error == 0) { > pr->ndpr_stateflags &= ~NDPRF_ONLINK; > > /* report the route deletion to the routing socket. */ > - if (rt != NULL) > - rt_sendmsg(rt, RTM_DELETE, ifp->if_rdomain); > + rt_sendmsg(rt, RTM_DELETE, ifp->if_rdomain); > > /* > * There might be the same prefix on another interface, > @@ -1946,6 +1946,12 @@ nd6_prefix_offlink(struct nd_prefix *pr) > } > } > } > + > + if (rt->rt_refcnt <= 0) { > + /* XXX: we should free the entry ourselves. */ > + rt->rt_refcnt++; > + rtfree(rt); > + } > } else { > /* XXX: can we still set the NDPRF_ONLINK flag? */ > nd6log((LOG_ERR, > @@ -1953,14 +1959,6 @@ nd6_prefix_offlink(struct nd_prefix *pr) > "%s/%d on %s (errno = %d)\n", > inet_ntop(AF_INET6, &sa6.sin6_addr, addr, sizeof(addr)), > pr->ndpr_plen, ifp->if_xname, error)); > - } > - > - if (rt != NULL) { > - if (rt->rt_refcnt <= 0) { > - /* XXX: we should free the entry ourselves. */ > - rt->rt_refcnt++; > - rtfree(rt); > - } > } > > return (error); > Index: share/man/man9/rtrequest1.9 > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/share/man/man9/rtrequest1.9,v > retrieving revision 1.3 > diff -u -p -r1.3 rtrequest1.9 > --- share/man/man9/rtrequest1.9 15 Oct 2014 11:58:13 -0000 1.3 > +++ share/man/man9/rtrequest1.9 18 Aug 2015 22:22:46 -0000 > @@ -80,13 +80,12 @@ and the requested action is > then a default priority based on the priority of the associated > interface is chosen. > .It Fa rtp > -Points to the cloning entry if the action is > -.Dv RTM_RESOLVE > -or, if the action is > -.Dv RTM_DELETE > -and it is non-NULL, a pointer to the removed entry is placed there. > -In this case, the caller must take care of releasing the returned entry by > -calling > +Must be non-NULL and point to the cloning entry if the action is > +.Dv RTM_RESOLVE . > +In all cases when no error is returned and it is non-NULL, a pointer > +to the deleted or added entry is placed there. > +Then the caller must take care of releasing the returned reference > +by calling > .Xr rtfree 9 . > .It Fa rtableid > The ID of the routing table to modify. >