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

Reply via email to