Re: rtsock refactoring

2017-01-23 Thread Claudio Jeker
On Tue, Jan 24, 2017 at 08:54:23AM +1000, Martin Pieuchot wrote:
> On 23/01/17(Mon) 01:18, Claudio Jeker wrote:
> > [...] 
> > Last bit for now. This is changing the reporting madness. It moves it in
> > its own function which is called after the big switch statement.
> > If you hit a bad error in the switch the code should eiter goto fail or
> > flush. 
> > The new function rt_report is actually constructing the rt message out of
> > a rtentry. It does not magically update the message that was passed in
> > from userland. I think this is much safer and works better.
> 
> [...]
> 
> > +   /*
> > +* From here on these vars need to be valid
> > +* rt, rtm, error, so, m, tableid, sa_family
> 
> Why not use KASSERT() rather than a comment?

I could not come up easily with proper KASSERT() statements.
Will ponder a bit more about this.

> 
> ok mpi@  either way .
> 

-- 
:wq Claudio



Re: rtsock refactoring

2017-01-23 Thread Claudio Jeker
On Mon, Jan 23, 2017 at 04:56:02PM +0100, Alexander Bluhm wrote:
> On Mon, Jan 23, 2017 at 01:18:05AM +0100, Claudio Jeker wrote:
> > Last bit for now. This is changing the reporting madness. It moves it in
> > its own function which is called after the big switch statement.
> > If you hit a bad error in the switch the code should eiter goto fail or
> > flush. 
> > The new function rt_report is actually constructing the rt message out of
> > a rtentry. It does not magically update the message that was passed in
> > from userland. I think this is much safer and works better.
> 
> OK bluhm@
> 
> > +rt_report(struct rtentry *rt, u_char type, int seq, int tableid)
> ...
> > +   ifp = if_get(rt->rt_ifidx);
> > +   if (ifp != NULL) {
> > +   info.rti_info[RTAX_IFP] = sdltosa(ifp->if_sadl);
> > +   info.rti_info[RTAX_IFA] = rt->rt_ifa->ifa_addr;
> > +   if (ifp->if_flags & IFF_POINTOPOINT)
> > +   info.rti_info[RTAX_BRD] = rt->rt_ifa->ifa_dstaddr;
> > +   }
> > +   if_put(ifp);
> 
> The if_put() could be in the if (ifp != NULL) block.
> 

While true, I like the symetry of if_get and if_put. Makes sure the if_put
is not lost in some other later code changes.

> >  flush:
> > +   if (rt)
> > +   rtfree(rt);
> 
> rtfree() checks (rt != NULL) itself.
> 

Thanks, will change that.

-- 
:wq Claudio



Re: rtsock refactoring

2017-01-23 Thread Martin Pieuchot
On 23/01/17(Mon) 01:18, Claudio Jeker wrote:
> [...] 
> Last bit for now. This is changing the reporting madness. It moves it in
> its own function which is called after the big switch statement.
> If you hit a bad error in the switch the code should eiter goto fail or
> flush. 
> The new function rt_report is actually constructing the rt message out of
> a rtentry. It does not magically update the message that was passed in
> from userland. I think this is much safer and works better.

[...]

> + /*
> +  * From here on these vars need to be valid
> +  * rt, rtm, error, so, m, tableid, sa_family

Why not use KASSERT() rather than a comment?

ok mpi@  either way .



Re: rtsock refactoring

2017-01-23 Thread Alexander Bluhm
On Mon, Jan 23, 2017 at 01:18:05AM +0100, Claudio Jeker wrote:
> Last bit for now. This is changing the reporting madness. It moves it in
> its own function which is called after the big switch statement.
> If you hit a bad error in the switch the code should eiter goto fail or
> flush. 
> The new function rt_report is actually constructing the rt message out of
> a rtentry. It does not magically update the message that was passed in
> from userland. I think this is much safer and works better.

OK bluhm@

> +rt_report(struct rtentry *rt, u_char type, int seq, int tableid)
...
> + ifp = if_get(rt->rt_ifidx);
> + if (ifp != NULL) {
> + info.rti_info[RTAX_IFP] = sdltosa(ifp->if_sadl);
> + info.rti_info[RTAX_IFA] = rt->rt_ifa->ifa_addr;
> + if (ifp->if_flags & IFF_POINTOPOINT)
> + info.rti_info[RTAX_BRD] = rt->rt_ifa->ifa_dstaddr;
> + }
> + if_put(ifp);

The if_put() could be in the if (ifp != NULL) block.

>  flush:
> + if (rt)
> + rtfree(rt);

rtfree() checks (rt != NULL) itself.



Re: rtsock refactoring

2017-01-23 Thread Alexander Bluhm
On Sat, Jan 21, 2017 at 01:28:02AM +0100, Claudio Jeker wrote:
> On Fri, Jan 20, 2017 at 02:51:52AM +0100, Claudio Jeker wrote:
> > I sent this diff out some time ago and would really like to get this in.
> > This is one step on makeing rtsock.c less of a hornets nest.
> > This reduces the side effects in route_output and simplifies some other
> > bits as well. For example route_input is less variadic and simpler.
> > 
> 
> Here is just the route_input change. Which should be easier to OK.
> Changed a few things based on input from bluhm@

OK bluhm@

> 
> -- 
> :wq Claudio
> 
> Index: net/rtsock.c
> ===
> RCS file: /cvs/src/sys/net/rtsock.c,v
> retrieving revision 1.214
> diff -u -p -r1.214 rtsock.c
> --- net/rtsock.c  20 Jan 2017 08:10:54 -  1.214
> +++ net/rtsock.c  21 Jan 2017 00:10:16 -
> @@ -92,7 +92,6 @@
>  
>  struct sockaddr  route_dst = { 2, PF_ROUTE, };
>  struct sockaddr  route_src = { 2, PF_ROUTE, };
> -struct sockproto route_proto = { PF_ROUTE, };
>  
>  struct walkarg {
>   int w_op, w_arg, w_given, w_needed, w_tmemsize;
> @@ -100,7 +99,7 @@ struct walkarg {
>  };
>  
>  int  route_ctloutput(int, struct socket *, int, int, struct mbuf **);
> -void route_input(struct mbuf *m0, ...);
> +void route_input(struct mbuf *m0, sa_family_t);
>  int  route_arp_conflict(struct rtentry *, struct rt_addrinfo *);
>  int  route_cleargateway(struct rtentry *, void *, unsigned int);
>  
> @@ -332,7 +331,7 @@ rt_senddesync(void *data)
>  }
>  
>  void
> -route_input(struct mbuf *m0, ...)
> +route_input(struct mbuf *m0, sa_family_t sa_family)
>  {
>   struct rawcb *rp;
>   struct routecb *rop;
> @@ -340,15 +339,10 @@ route_input(struct mbuf *m0, ...)
>   struct mbuf *m = m0;
>   int s, sockets = 0;
>   struct socket *last = NULL;
> - va_list ap;
> - struct sockproto *proto;
>   struct sockaddr *sosrc, *sodst;
>   
> - va_start(ap, m0);
> - proto = va_arg(ap, struct sockproto *);
> - sosrc = va_arg(ap, struct sockaddr *);
> - sodst = va_arg(ap, struct sockaddr *);
> - va_end(ap);
> + sosrc = _src;
> + sodst = _dst;
>  
>   /* ensure that we can access the rtm_type via mtod() */
>   if (m->m_len < offsetof(struct rt_msghdr, rtm_type) + 1) {
> @@ -359,10 +353,16 @@ route_input(struct mbuf *m0, ...)
>   LIST_FOREACH(rp, , rcb_list) {
>   if (rp->rcb_socket->so_state & SS_CANTRCVMORE)
>   continue;
> - if (rp->rcb_proto.sp_family != proto->sp_family)
> + if (rp->rcb_proto.sp_family != PF_ROUTE)
>   continue;
> - if (rp->rcb_proto.sp_protocol && proto->sp_protocol &&
> - rp->rcb_proto.sp_protocol != proto->sp_protocol)
> + /*
> +  * If route socket is bound to an address family only send
> +  * messages that match the address family. Address family
> +  * agnostic messages are always send.
> +  */
> + if (rp->rcb_proto.sp_protocol != AF_UNSPEC &&
> + sa_family != AF_UNSPEC &&
> + rp->rcb_proto.sp_protocol != sa_family)
>   continue;
>   /*
>* We assume the lower level routines have
> @@ -953,8 +953,6 @@ flush:
>   rtm->rtm_flags |= RTF_DONE;
>   }
>   }
> - if (info.rti_info[RTAX_DST])
> - route_proto.sp_protocol = info.rti_info[RTAX_DST]->sa_family;
>   if (rt)
>   rtfree(rt);
>  
> @@ -970,9 +968,8 @@ fail:
>   }
>   /* There is another listener, so construct message */
>   rp = sotorawcb(so);
> - }
> - if (rp)
>   rp->rcb_proto.sp_family = 0; /* Avoid us */
> + }
>   if (rtm) {
>   if (m_copyback(m, 0, rtm->rtm_msglen, rtm, M_NOWAIT)) {
>   m_freem(m);
> @@ -982,9 +979,10 @@ fail:
>   free(rtm, M_RTABLE, 0);
>   }
>   if (m)
> - route_input(m, _proto, _src, _dst);
> + route_input(m, info.rti_info[RTAX_DST] ?
> + info.rti_info[RTAX_DST]->sa_family : AF_UNSPEC);
>   if (rp)
> - rp->rcb_proto.sp_family = PF_ROUTE;
> + rp->rcb_proto.sp_family = PF_ROUTE; /* Readd us */
>  
>   return (error);
>  }
> @@ -1056,7 +1054,6 @@ rt_setmetrics(u_long which, const struct
>  
>   out->rmx_expire = expire;
>   }
> - /* RTV_PRIORITY handled before */
>  }
>  
>  void
> @@ -1258,11 +1255,7 @@ rt_missmsg(int type, struct rt_addrinfo 
>   rtm->rtm_tableid = tableid;
>   rtm->rtm_addrs = rtinfo->rti_addrs;
>   rtm->rtm_index = ifidx;
> - if (sa == NULL)
> - route_proto.sp_protocol = 0;
> - else
> - route_proto.sp_protocol = sa->sa_family;
> - route_input(m, _proto, _src, 

Re: rtsock refactoring

2017-01-22 Thread Claudio Jeker
On Sat, Jan 21, 2017 at 07:31:20AM +0100, Claudio Jeker wrote:
> On Sat, Jan 21, 2017 at 01:28:02AM +0100, Claudio Jeker wrote:
> > On Fri, Jan 20, 2017 at 02:51:52AM +0100, Claudio Jeker wrote:
> > > I sent this diff out some time ago and would really like to get this in.
> > > This is one step on makeing rtsock.c less of a hornets nest.
> > > This reduces the side effects in route_output and simplifies some other
> > > bits as well. For example route_input is less variadic and simpler.
> > > 
> > 
> > Here is just the route_input change. Which should be easier to OK.
> > Changed a few things based on input from bluhm@
> > 
> 
> Next piece of the puzzle. Cleanup the error handling a bit.
> If the route message is not valid (syntactically or also because it
> references bad things) fail without broadcasting this message to all
> listeners. So make sure that until the info struct has been checked we only
> use goto fail instead of goto flush.
> While there remove some redundant checks which are not needed.
> 

Last bit for now. This is changing the reporting madness. It moves it in
its own function which is called after the big switch statement.
If you hit a bad error in the switch the code should eiter goto fail or
flush. 
The new function rt_report is actually constructing the rt message out of
a rtentry. It does not magically update the message that was passed in
from userland. I think this is much safer and works better.

The diff is maybe hard to read because big chunks of code got moved
around.

With this the side effects happening in route_output are mostly gone I
think.
-- 
:wq Claudio

Index: net/rtsock.c
===
RCS file: /cvs/src/sys/net/rtsock.c,v
retrieving revision 1.216
diff -u -p -r1.216 rtsock.c
--- net/rtsock.c22 Jan 2017 04:31:02 -  1.216
+++ net/rtsock.c22 Jan 2017 05:02:42 -
@@ -459,30 +459,94 @@ route_input(struct mbuf *m0, sa_family_t
m_freem(m);
 }
 
+struct rt_msghdr *
+rt_report(struct rtentry *rt, u_char type, int seq, int tableid)
+{
+   struct rt_msghdr*rtm;
+   struct rt_addrinfo   info;
+   struct sockaddr_rtlabel  sa_rl;
+   struct sockaddr_in6  sa_mask;
+#ifdef BFD
+   struct sockaddr_bfd  sa_bfd;
+#endif
+#ifdef MPLS
+   struct sockaddr_mpls sa_mpls;
+#endif
+   struct ifnet*ifp = NULL;
+   int  len;
+
+   bzero(, sizeof(info));
+   info.rti_info[RTAX_DST] = rt_key(rt);
+   info.rti_info[RTAX_GATEWAY] = rt->rt_gateway;
+   info.rti_info[RTAX_NETMASK] = rt_plen2mask(rt, _mask);
+   info.rti_info[RTAX_LABEL] = rtlabel_id2sa(rt->rt_labelid, _rl);
+#ifdef BFD
+   if (rt->rt_flags & RTF_BFD)
+   info.rti_info[RTAX_BFD] = bfd2sa(rt, _bfd);
+#endif
+#ifdef MPLS
+   if (rt->rt_flags & RTF_MPLS) {
+   bzero(_mpls, sizeof(sa_mpls));
+   sa_mpls.smpls_family = AF_MPLS;
+   sa_mpls.smpls_len = sizeof(sa_mpls);
+   sa_mpls.smpls_label = ((struct rt_mpls *)
+   rt->rt_llinfo)->mpls_label;
+   info.rti_info[RTAX_SRC] = (struct sockaddr *)_mpls;
+   info.rti_mpls = ((struct rt_mpls *)
+   rt->rt_llinfo)->mpls_operation;
+   }
+#endif
+   ifp = if_get(rt->rt_ifidx);
+   if (ifp != NULL) {
+   info.rti_info[RTAX_IFP] = sdltosa(ifp->if_sadl);
+   info.rti_info[RTAX_IFA] = rt->rt_ifa->ifa_addr;
+   if (ifp->if_flags & IFF_POINTOPOINT)
+   info.rti_info[RTAX_BRD] = rt->rt_ifa->ifa_dstaddr;
+   }
+   if_put(ifp);
+   /* RTAX_GENMASK, RTAX_AUTHOR, RTAX_SRCMASK ignored */
+
+   /* build new route message */
+   len = rt_msg2(type, RTM_VERSION, , NULL, NULL);
+   /* XXX why can't we wait? Should be process context... */
+   rtm = malloc(len, M_RTABLE, M_NOWAIT | M_ZERO);
+   if (rtm == NULL)
+   return NULL;
+
+   rt_msg2(type, RTM_VERSION, , (caddr_t)rtm, NULL);
+   rtm->rtm_type = type;
+   rtm->rtm_index = rt->rt_ifidx;
+   rtm->rtm_tableid = tableid;
+   rtm->rtm_priority = rt->rt_priority & RTP_MASK;
+   rtm->rtm_flags = rt->rt_flags;
+   rtm->rtm_pid = curproc->p_p->ps_pid;
+   rtm->rtm_seq = seq;
+   rt_getmetrics(>rt_rmx, >rtm_rmx);
+   rtm->rtm_addrs = info.rti_addrs;
+#ifdef MPLS
+   rtm->rtm_mpls = info.rti_mpls;
+#endif
+   return rtm;
+}
+
 int
 route_output(struct mbuf *m, ...)
 {
struct rt_msghdr*rtm = NULL;
struct rtentry  *rt = NULL;
-   struct rtentry  *saved_nrt = NULL;
struct rt_addrinfo   info;
-   int  plen, len, newgate = 0, error = 0;
+   int  plen, len, seq, newgate = 0, error = 0;
struct ifnet*ifp = NULL;
struct ifaddr   *ifa = NULL;
struct 

Re: rtsock refactoring

2017-01-20 Thread Claudio Jeker
On Sat, Jan 21, 2017 at 01:28:02AM +0100, Claudio Jeker wrote:
> On Fri, Jan 20, 2017 at 02:51:52AM +0100, Claudio Jeker wrote:
> > I sent this diff out some time ago and would really like to get this in.
> > This is one step on makeing rtsock.c less of a hornets nest.
> > This reduces the side effects in route_output and simplifies some other
> > bits as well. For example route_input is less variadic and simpler.
> > 
> 
> Here is just the route_input change. Which should be easier to OK.
> Changed a few things based on input from bluhm@
> 

Next piece of the puzzle. Cleanup the error handling a bit.
If the route message is not valid (syntactically or also because it
references bad things) fail without broadcasting this message to all
listeners. So make sure that until the info struct has been checked we only
use goto fail instead of goto flush.
While there remove some redundant checks which are not needed.

OK?
-- 
:wq Claudio

Index: net/rtsock.c
===
RCS file: /cvs/src/sys/net/rtsock.c,v
retrieving revision 1.215
diff -u -p -r1.215 rtsock.c
--- net/rtsock.c21 Jan 2017 03:44:46 -  1.215
+++ net/rtsock.c21 Jan 2017 06:23:02 -
@@ -488,7 +488,6 @@ route_output(struct mbuf *m, ...)
so = va_arg(ap, struct socket *);
va_end(ap);
 
-   info.rti_info[RTAX_DST] = NULL; /* for error handling (goto flush) */
if (m == NULL || ((m->m_len < sizeof(int32_t)) &&
(m = m_pullup(m, sizeof(int32_t))) == 0))
return (ENOBUFS);
@@ -555,10 +554,10 @@ route_output(struct mbuf *m, ...)
if (!rtable_exists(tableid)) {
if (rtm->rtm_type == RTM_ADD) {
if ((error = rtable_add(tableid)) != 0)
-   goto flush;
+   goto fail;
} else {
error = EINVAL;
-   goto flush;
+   goto fail;
}
}
 
@@ -598,7 +597,7 @@ route_output(struct mbuf *m, ...)
info.rti_info[RTAX_GATEWAY]->sa_family >= AF_MAX) ||
info.rti_info[RTAX_GENMASK] != NULL) {
error = EINVAL;
-   goto flush;
+   goto fail;
}
 #ifdef MPLS
info.rti_mpls = rtm->rtm_mpls;
@@ -610,6 +609,11 @@ route_output(struct mbuf *m, ...)
info.rti_flags |= RTF_LLINFO;
}
 
+   /*
+* Do not use goto flush before this point since the message itself
+* may be not consistent and could cause unexpected behaviour in other
+* userland clients. Use goto fail instead.
+*/
switch (rtm->rtm_type) {
case RTM_ADD:
if (info.rti_info[RTAX_GATEWAY] == NULL) {
@@ -647,11 +651,6 @@ route_output(struct mbuf *m, ...)
}
break;
case RTM_DELETE:
-   if (!rtable_exists(tableid)) {
-   error = EAFNOSUPPORT;
-   goto flush;
-   }
-
rt = rtable_lookup(tableid, info.rti_info[RTAX_DST],
info.rti_info[RTAX_NETMASK], info.rti_info[RTAX_GATEWAY],
prio);
@@ -690,10 +689,6 @@ route_output(struct mbuf *m, ...)
goto report;
break;
case RTM_GET:
-   if (!rtable_exists(tableid)) {
-   error = EAFNOSUPPORT;
-   goto flush;
-   }
rt = rtable_lookup(tableid, info.rti_info[RTAX_DST],
info.rti_info[RTAX_NETMASK], info.rti_info[RTAX_GATEWAY],
prio);
@@ -764,11 +759,6 @@ report:
break;
case RTM_CHANGE:
case RTM_LOCK:
-   if (!rtable_exists(tableid)) {
-   error = EAFNOSUPPORT;
-   goto flush;
-   }
-
rt = rtable_lookup(tableid, info.rti_info[RTAX_DST],
info.rti_info[RTAX_NETMASK], info.rti_info[RTAX_GATEWAY],
prio);



Re: rtsock refactoring

2017-01-20 Thread Claudio Jeker
On Fri, Jan 20, 2017 at 02:51:52AM +0100, Claudio Jeker wrote:
> I sent this diff out some time ago and would really like to get this in.
> This is one step on makeing rtsock.c less of a hornets nest.
> This reduces the side effects in route_output and simplifies some other
> bits as well. For example route_input is less variadic and simpler.
> 

Here is just the route_input change. Which should be easier to OK.
Changed a few things based on input from bluhm@

-- 
:wq Claudio

Index: net/rtsock.c
===
RCS file: /cvs/src/sys/net/rtsock.c,v
retrieving revision 1.214
diff -u -p -r1.214 rtsock.c
--- net/rtsock.c20 Jan 2017 08:10:54 -  1.214
+++ net/rtsock.c21 Jan 2017 00:10:16 -
@@ -92,7 +92,6 @@
 
 struct sockaddrroute_dst = { 2, PF_ROUTE, };
 struct sockaddrroute_src = { 2, PF_ROUTE, };
-struct sockproto   route_proto = { PF_ROUTE, };
 
 struct walkarg {
int w_op, w_arg, w_given, w_needed, w_tmemsize;
@@ -100,7 +99,7 @@ struct walkarg {
 };
 
 introute_ctloutput(int, struct socket *, int, int, struct mbuf **);
-void   route_input(struct mbuf *m0, ...);
+void   route_input(struct mbuf *m0, sa_family_t);
 introute_arp_conflict(struct rtentry *, struct rt_addrinfo *);
 introute_cleargateway(struct rtentry *, void *, unsigned int);
 
@@ -332,7 +331,7 @@ rt_senddesync(void *data)
 }
 
 void
-route_input(struct mbuf *m0, ...)
+route_input(struct mbuf *m0, sa_family_t sa_family)
 {
struct rawcb *rp;
struct routecb *rop;
@@ -340,15 +339,10 @@ route_input(struct mbuf *m0, ...)
struct mbuf *m = m0;
int s, sockets = 0;
struct socket *last = NULL;
-   va_list ap;
-   struct sockproto *proto;
struct sockaddr *sosrc, *sodst;

-   va_start(ap, m0);
-   proto = va_arg(ap, struct sockproto *);
-   sosrc = va_arg(ap, struct sockaddr *);
-   sodst = va_arg(ap, struct sockaddr *);
-   va_end(ap);
+   sosrc = _src;
+   sodst = _dst;
 
/* ensure that we can access the rtm_type via mtod() */
if (m->m_len < offsetof(struct rt_msghdr, rtm_type) + 1) {
@@ -359,10 +353,16 @@ route_input(struct mbuf *m0, ...)
LIST_FOREACH(rp, , rcb_list) {
if (rp->rcb_socket->so_state & SS_CANTRCVMORE)
continue;
-   if (rp->rcb_proto.sp_family != proto->sp_family)
+   if (rp->rcb_proto.sp_family != PF_ROUTE)
continue;
-   if (rp->rcb_proto.sp_protocol && proto->sp_protocol &&
-   rp->rcb_proto.sp_protocol != proto->sp_protocol)
+   /*
+* If route socket is bound to an address family only send
+* messages that match the address family. Address family
+* agnostic messages are always send.
+*/
+   if (rp->rcb_proto.sp_protocol != AF_UNSPEC &&
+   sa_family != AF_UNSPEC &&
+   rp->rcb_proto.sp_protocol != sa_family)
continue;
/*
 * We assume the lower level routines have
@@ -953,8 +953,6 @@ flush:
rtm->rtm_flags |= RTF_DONE;
}
}
-   if (info.rti_info[RTAX_DST])
-   route_proto.sp_protocol = info.rti_info[RTAX_DST]->sa_family;
if (rt)
rtfree(rt);
 
@@ -970,9 +968,8 @@ fail:
}
/* There is another listener, so construct message */
rp = sotorawcb(so);
-   }
-   if (rp)
rp->rcb_proto.sp_family = 0; /* Avoid us */
+   }
if (rtm) {
if (m_copyback(m, 0, rtm->rtm_msglen, rtm, M_NOWAIT)) {
m_freem(m);
@@ -982,9 +979,10 @@ fail:
free(rtm, M_RTABLE, 0);
}
if (m)
-   route_input(m, _proto, _src, _dst);
+   route_input(m, info.rti_info[RTAX_DST] ?
+   info.rti_info[RTAX_DST]->sa_family : AF_UNSPEC);
if (rp)
-   rp->rcb_proto.sp_family = PF_ROUTE;
+   rp->rcb_proto.sp_family = PF_ROUTE; /* Readd us */
 
return (error);
 }
@@ -1056,7 +1054,6 @@ rt_setmetrics(u_long which, const struct
 
out->rmx_expire = expire;
}
-   /* RTV_PRIORITY handled before */
 }
 
 void
@@ -1258,11 +1255,7 @@ rt_missmsg(int type, struct rt_addrinfo 
rtm->rtm_tableid = tableid;
rtm->rtm_addrs = rtinfo->rti_addrs;
rtm->rtm_index = ifidx;
-   if (sa == NULL)
-   route_proto.sp_protocol = 0;
-   else
-   route_proto.sp_protocol = sa->sa_family;
-   route_input(m, _proto, _src, _dst);
+   route_input(m, sa ? sa->sa_family : AF_UNSPEC);
 }
 
 /*
@@ -1287,8 +1280,7 @@ rt_ifmsg(struct ifnet *ifp)
ifm->ifm_xflags = ifp->if_xflags;

rtsock refactoring

2017-01-19 Thread Claudio Jeker
I sent this diff out some time ago and would really like to get this in.
This is one step on makeing rtsock.c less of a hornets nest.
This reduces the side effects in route_output and simplifies some other
bits as well. For example route_input is less variadic and simpler.

Anyone couragous enough to send me an OK?
-- 
:wq Claudio

Index: net/rtsock.c
===
RCS file: /cvs/src/sys/net/rtsock.c,v
retrieving revision 1.213
diff -u -p -r1.213 rtsock.c
--- net/rtsock.c19 Jan 2017 23:18:29 -  1.213
+++ net/rtsock.c20 Jan 2017 01:37:33 -
@@ -92,7 +92,6 @@
 
 struct sockaddrroute_dst = { 2, PF_ROUTE, };
 struct sockaddrroute_src = { 2, PF_ROUTE, };
-struct sockproto   route_proto = { PF_ROUTE, };
 
 struct walkarg {
int w_op, w_arg, w_given, w_needed, w_tmemsize;
@@ -100,7 +99,7 @@ struct walkarg {
 };
 
 introute_ctloutput(int, struct socket *, int, int, struct mbuf **);
-void   route_input(struct mbuf *m0, ...);
+void   route_input(struct mbuf *m0, unsigned short);
 introute_arp_conflict(struct rtentry *, struct rt_addrinfo *);
 introute_cleargateway(struct rtentry *, void *, unsigned int);
 
@@ -331,7 +330,7 @@ rt_senddesync(void *data)
 }
 
 void
-route_input(struct mbuf *m0, ...)
+route_input(struct mbuf *m0, unsigned short sa_family)
 {
struct rawcb *rp;
struct routecb *rop;
@@ -339,15 +338,10 @@ route_input(struct mbuf *m0, ...)
struct mbuf *m = m0;
int s, sockets = 0;
struct socket *last = NULL;
-   va_list ap;
-   struct sockproto *proto;
struct sockaddr *sosrc, *sodst;

-   va_start(ap, m0);
-   proto = va_arg(ap, struct sockproto *);
-   sosrc = va_arg(ap, struct sockaddr *);
-   sodst = va_arg(ap, struct sockaddr *);
-   va_end(ap);
+   sosrc = _src;
+   sodst = _src;
 
/* ensure that we can access the rtm_type via mtod() */
if (m->m_len < offsetof(struct rt_msghdr, rtm_type) + 1) {
@@ -358,10 +352,15 @@ route_input(struct mbuf *m0, ...)
LIST_FOREACH(rp, , rcb_list) {
if (rp->rcb_socket->so_state & SS_CANTRCVMORE)
continue;
-   if (rp->rcb_proto.sp_family != proto->sp_family)
+   if (rp->rcb_proto.sp_family != PF_ROUTE)
continue;
-   if (rp->rcb_proto.sp_protocol && proto->sp_protocol &&
-   rp->rcb_proto.sp_protocol != proto->sp_protocol)
+   /*
+* If route socket is bound to an address family only send
+* messages that match the address family. Address family
+* agnostic messages are always send.
+*/
+   if (rp->rcb_proto.sp_protocol != 0 && sa_family != 0 &&
+   rp->rcb_proto.sp_protocol != sa_family)
continue;
/*
 * We assume the lower level routines have
@@ -458,36 +457,99 @@ route_input(struct mbuf *m0, ...)
m_freem(m);
 }
 
+struct rt_msghdr *
+rt_report(struct rtentry *rt, u_char type, int seq, int tableid)
+{
+   struct rt_msghdr*rtm;
+   struct rt_addrinfo   info;
+   struct sockaddr_rtlabel  sa_rl;
+   struct sockaddr_in6  sa_mask;
+#ifdef BFD
+   struct sockaddr_bfd  sa_bfd;
+#endif
+#ifdef MPLS
+   struct sockaddr_mpls sa_mpls;
+#endif
+   struct ifnet*ifp = NULL;
+   int  len;
+
+   bzero(, sizeof(info));
+   info.rti_info[RTAX_DST] = rt_key(rt);
+   info.rti_info[RTAX_GATEWAY] = rt->rt_gateway;
+   info.rti_info[RTAX_NETMASK] = rt_plen2mask(rt, _mask);
+   info.rti_info[RTAX_LABEL] = rtlabel_id2sa(rt->rt_labelid, _rl);
+#ifdef BFD
+   if (rt->rt_flags & RTF_BFD)
+   info.rti_info[RTAX_BFD] = bfd2sa(rt, _bfd);
+#endif
+#ifdef MPLS
+   if (rt->rt_flags & RTF_MPLS) {
+   bzero(_mpls, sizeof(sa_mpls));
+   sa_mpls.smpls_family = AF_MPLS;
+   sa_mpls.smpls_len = sizeof(sa_mpls);
+   sa_mpls.smpls_label = ((struct rt_mpls *)
+   rt->rt_llinfo)->mpls_label;
+   info.rti_info[RTAX_SRC] = (struct sockaddr *)_mpls;
+   info.rti_mpls = ((struct rt_mpls *)
+   rt->rt_llinfo)->mpls_operation;
+   }
+#endif
+   ifp = if_get(rt->rt_ifidx);
+   if (ifp != NULL) {
+   info.rti_info[RTAX_IFP] = sdltosa(ifp->if_sadl);
+   info.rti_info[RTAX_IFA] = rt->rt_ifa->ifa_addr;
+   if (ifp->if_flags & IFF_POINTOPOINT)
+   info.rti_info[RTAX_BRD] = rt->rt_ifa->ifa_dstaddr;
+   }
+   if_put(ifp);
+   /* RTAX_GENMASK, RTAX_AUTHOR, RTAX_SRCMASK ignored */
+
+   /* build new route message */
+   len = rt_msg2(type, RTM_VERSION, , NULL, NULL);
+   /* XXX