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.
>
