On Wed, Feb 04, 2015 at 02:15:22AM +0100, Martin Pieuchot wrote:
> 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.
>
I just don't trust myself to figure out all the crazy cases people come up
with... At least the history with the routing code teached me that there
is almost always on strange edge case that was missed.
Since we are heading into lock very soon I'm more conservative about
routing changes happening right now. I still think this is OK and it
should go in. I hope to have time to test it on the flight.
> >
> > > 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
> >
>
--
:wq Claudio
