On 03/02/15(Tue) 16:25, Claudio Jeker wrote: > On Tue, Feb 03, 2015 at 10:39:35AM +0100, Martin Pieuchot wrote: > > Diff below changes rt_mpath_conflict() to no longer rely on a fully > > initialized rtentry. Right now it makes things prettier when adding > > a new route entry but later it will also help to dissociate "struct > > radix_node" from "struct rtentry". > > > > route(8) regression tests checking for conflicts are happy with this. > > > > Ok? > > Reads fine. Not sure if the regress test is enough for this.
I believe it covers all the different conflict cases, but if you think something is not covered we can always add new tests. > > > Index: net/radix_mpath.c > > =================================================================== > > RCS file: /cvs/src/sys/net/radix_mpath.c,v > > retrieving revision 1.27 > > diff -u -p -r1.27 radix_mpath.c > > --- net/radix_mpath.c 19 Dec 2014 17:14:40 -0000 1.27 > > +++ net/radix_mpath.c 3 Feb 2015 09:20:38 -0000 > > @@ -204,16 +204,15 @@ rt_mpath_matchgate(struct rtentry *rt, s > > * check if we have the same key/mask/gateway on the table already. > > */ > > int > > -rt_mpath_conflict(struct radix_node_head *rnh, struct rtentry *rt, > > - struct sockaddr *netmask, int mpathok) > > +rt_mpath_conflict(struct radix_node_head *rnh, struct sockaddr *dst, > > + struct sockaddr *netmask, struct sockaddr *gate, u_int8_t prio, int > > mpathok) > > { > > - struct radix_node *rn, *rn1; > > + struct radix_node *rn1; > > struct rtentry *rt1; > > char *p, *q, *eq; > > int same, l, skip; > > > > - rn = (struct radix_node *)rt; > > - rn1 = rnh->rnh_lookup(rt_key(rt), netmask, rnh); > > + rn1 = rnh->rnh_lookup(dst, netmask, rnh); > > if (!rn1 || rn1->rn_flags & RNF_ROOT) > > return 0; > > > > @@ -224,8 +223,8 @@ rt_mpath_conflict(struct radix_node_head > > rt1 = (struct rtentry *)rn1; > > > > /* compare key. */ > > - if (rt_key(rt1)->sa_len != rt_key(rt)->sa_len || > > - bcmp(rt_key(rt1), rt_key(rt), rt_key(rt1)->sa_len)) > > + if (rt_key(rt1)->sa_len != dst->sa_len || > > + bcmp(rt_key(rt1), dst, rt_key(rt1)->sa_len)) > > goto different; > > > > /* key was the same. compare netmask. hairy... */ > > @@ -277,11 +276,11 @@ rt_mpath_conflict(struct radix_node_head > > } > > > > maskmatched: > > - if (!mpathok && rt1->rt_priority == rt->rt_priority) > > + if (!mpathok && rt1->rt_priority == prio) > > return EEXIST; > > > > /* key/mask were the same. compare gateway for all multipaths */ > > - if (rt_mpath_matchgate(rt1, rt->rt_gateway, rt->rt_priority)) > > + if (rt_mpath_matchgate(rt1, gate, prio)) > > /* all key/mask/gateway are the same. conflicting entry. */ > > return EEXIST; > > > > Index: net/radix_mpath.h > > =================================================================== > > RCS file: /cvs/src/sys/net/radix_mpath.h,v > > retrieving revision 1.14 > > diff -u -p -r1.14 radix_mpath.h > > --- net/radix_mpath.h 25 Nov 2014 14:50:46 -0000 1.14 > > +++ net/radix_mpath.h 3 Feb 2015 09:20:07 -0000 > > @@ -54,8 +54,8 @@ void rn_mpath_adj_mpflag(struct radix_no > > int rn_mpath_active_count(struct radix_node *); > > struct rtentry *rt_mpath_matchgate(struct rtentry *, struct sockaddr *, > > u_int8_t); > > -int rt_mpath_conflict(struct radix_node_head *, struct rtentry *, > > - struct sockaddr *, int); > > +int rt_mpath_conflict(struct radix_node_head *, struct sockaddr *, > > + struct sockaddr *, struct sockaddr *, u_int8_t, int); > > struct rtentry *rtalloc_mpath(struct sockaddr *, u_int32_t *, u_int); > > int rn_mpath_inithead(void **, int); > > #endif /* _KERNEL */ > > Index: net/route.c > > =================================================================== > > RCS file: /cvs/src/sys/net/route.c,v > > retrieving revision 1.203 > > diff -u -p -r1.203 route.c > > --- net/route.c 28 Jan 2015 22:10:13 -0000 1.203 > > +++ net/route.c 3 Feb 2015 09:23:26 -0000 > > @@ -826,15 +826,25 @@ rtrequest1(int req, struct rt_addrinfo * > > if (info->rti_ifa == NULL && (error = rt_getifa(info, tableid))) > > return (error); > > ifa = info->rti_ifa; > > + if (prio == 0) > > + prio = ifa->ifa_ifp->if_priority + RTP_STATIC; > > +#ifndef SMALL_KERNEL > > + if (rn_mpath_capable(rnh)) { > > + /* do not permit exactly the same dst/mask/gw pair */ > > + if (rt_mpath_conflict(rnh, info->rti_info[RTAX_DST], > > + info->rti_info[RTAX_NETMASK], > > + info->rti_info[RTAX_GATEWAY], prio, > > + info->rti_flags & RTF_MPATH)) { > > + return (EEXIST); > > + } > > + } > > +#endif > > rt = pool_get(&rtentry_pool, PR_NOWAIT | PR_ZERO); > > if (rt == NULL) > > return (ENOBUFS); > > > > rt->rt_flags = info->rti_flags; > > rt->rt_tableid = tableid; > > - > > - if (prio == 0) > > - prio = ifa->ifa_ifp->if_priority + RTP_STATIC; > > rt->rt_priority = prio; /* init routing priority */ > > LIST_INIT(&rt->rt_timer); > > if ((error = rt_setgate(rt, info->rti_info[RTAX_DST], > > @@ -851,16 +861,6 @@ rtrequest1(int req, struct rt_addrinfo * > > info->rti_info[RTAX_DST]->sa_len); > > #ifndef SMALL_KERNEL > > if (rn_mpath_capable(rnh)) { > > - /* do not permit exactly the same dst/mask/gw pair */ > > - if (rt_mpath_conflict(rnh, rt, > > - info->rti_info[RTAX_NETMASK], > > - info->rti_flags & RTF_MPATH)) { > > - if (rt->rt_gwroute) > > - rtfree(rt->rt_gwroute); > > - free(rt_key(rt), M_RTABLE, 0); > > - pool_put(&rtentry_pool, rt); > > - return (EEXIST); > > - } > > /* check the link state since the table supports it */ > > if (LINK_STATE_IS_UP(ifa->ifa_ifp->if_link_state) && > > ifa->ifa_ifp->if_flags & IFF_UP) > > > > -- > :wq Claudio >