Re: Sending route messages for local routes or cloning routes

2015-01-12 Thread Martin Pieuchot 
On 07/01/15(Wed) 19:00, Florian Riehm wrote:
 Hi Martin,
 
 Thanks for your diff! Regardless of my problem it makes our code
 more clear. The loop in rt_newaddrmsg() was ugly.
 
  
  Here's a diff that should generate a RTM_ADD message for every CLONING
  route added while keeping the existing RTM_NEWADDR/RTM_DELADDR logic.
  
  dhclient(8) is happy with this change, does it fix your use case too?
 
 There is a small bug in your diff of route.c, because rt_ifa_del() is 
 never called with flag RTF_CLONING, so ospfd doesn't notice if an address
 gets deleted.
 
 I was thinking about this three options to fix the problem:
 1.) Check with rt-rt_flags instead of flags for RTF_CLONING flag:
 -if (flags  (RTF_LOCAL|RTF_CLONING))
 +if (rt-rt_flags  (RTF_LOCAL|RTF_CLONING))
 
 2) Add RTF_CLONING to flags argument at certain calls of rt_ifa_add() /
 rt_ifa_delete()
 
 3.) Remove the check completely and call always rt_sendmsg()
 
 At the moment I would prefer the 3. solution. After rtrequest1() has been
 called, we check for error and check if rt ist not NULL. If this conditions 
 are
 true we have added or deleted a route. Why should we not send a route message
 then? 
 Below an updated diff for route.c with my fix.

I agree with you, we should always send RTM_{ADD,DELETE} messages.

Complete diff below, anybody wants to ok it?

Index: net/route.c
===
RCS file: /home/ncvs/src/sys/net/route.c,v
retrieving revision 1.198
diff -u -p -r1.198 route.c
--- net/route.c 8 Jan 2015 15:05:44 -   1.198
+++ net/route.c 12 Jan 2015 13:53:02 -
@@ -382,11 +382,13 @@ void
 rt_sendmsg(struct rtentry *rt, int cmd, u_int rtableid)
 {
struct rt_addrinfo info;
+   struct sockaddr_rtlabel sa_rl;
 
-   bzero(info, sizeof(info));
+   memset(info, 0, sizeof(info));
info.rti_info[RTAX_DST] = rt_key(rt);
info.rti_info[RTAX_GATEWAY] = rt-rt_gateway;
info.rti_info[RTAX_NETMASK] = rt_mask(rt);
+   info.rti_info[RTAX_LABEL] = rtlabel_id2sa(rt-rt_labelid, sa_rl);
if (rt-rt_ifp != NULL) {
info.rti_info[RTAX_IFP] =(struct sockaddr *)rt-rt_ifp-if_sadl;
info.rti_info[RTAX_IFA] = rt-rt_ifa-ifa_addr;
@@ -1138,7 +1140,8 @@ rt_ifa_add(struct ifaddr *ifa, int flags
 * userland that a new address has been added.
 */
if (flags  RTF_LOCAL)
-   rt_newaddrmsg(RTM_ADD, ifa, error, nrt);
+   rt_sendaddrmsg(nrt, RTM_NEWADDR);
+   rt_sendmsg(nrt, RTM_ADD, rtableid);
}
return (error);
 }
@@ -1193,7 +1196,8 @@ rt_ifa_del(struct ifaddr *ifa, int flags
error = rtrequest1(RTM_DELETE, info, prio, nrt, rtableid);
if (error == 0  (rt = nrt) != NULL) {
if (flags  RTF_LOCAL)
-   rt_newaddrmsg(RTM_DELETE, ifa, error, nrt);
+   rt_sendaddrmsg(nrt, RTM_DELADDR);
+   rt_sendmsg(nrt, RTM_DELETE, rtableid);
if (rt-rt_refcnt = 0) {
rt-rt_refcnt++;
rtfree(rt);
Index: net/route.h
===
RCS file: /home/ncvs/src/sys/net/route.h,v
retrieving revision 1.103
diff -u -p -r1.103 route.h
--- net/route.h 8 Jan 2015 15:05:44 -   1.103
+++ net/route.h 12 Jan 2015 13:53:02 -
@@ -357,9 +357,9 @@ void rt_ifannouncemsg(struct ifnet *, i
 voidrt_maskedcopy(struct sockaddr *,
struct sockaddr *, struct sockaddr *);
 voidrt_sendmsg(struct rtentry *, int, u_int);
+voidrt_sendaddrmsg(struct rtentry *, int);
 voidrt_missmsg(int, struct rt_addrinfo *, int, struct ifnet *, int,
u_int);
-voidrt_newaddrmsg(int, struct ifaddr *, int, struct rtentry *);
 int rt_setgate(struct rtentry *, struct sockaddr *,
struct sockaddr *, u_int);
 int rt_checkgate(struct ifnet *, struct rtentry *, struct sockaddr *,
Index: net/rtsock.c
===
RCS file: /home/ncvs/src/sys/net/rtsock.c,v
retrieving revision 1.155
diff -u -p -r1.155 rtsock.c
--- net/rtsock.c19 Dec 2014 18:57:17 -  1.155
+++ net/rtsock.c12 Jan 2015 13:53:02 -
@@ -1137,70 +1137,36 @@ rt_ifmsg(struct ifnet *ifp)
  * copies of it.
  */
 void
-rt_newaddrmsg(int cmd, struct ifaddr *ifa, int error, struct rtentry *rt)
+rt_sendaddrmsg(struct rtentry *rt, int cmd)
 {
-   struct rt_addrinfo   info;
-   struct sockaddr *sa = NULL;
-   int  pass;
-   struct mbuf *m = NULL;
+   struct ifaddr   *ifa = rt-rt_ifa;
struct ifnet*ifp = ifa-ifa_ifp;
+   struct mbuf *m = NULL;
+   struct rt_addrinfo   info;
+   struct ifa_msghdr   *ifam;
 
if (route_cb.any_count == 0)
return;
-

Re: Sending route messages for local routes or cloning routes

2015-01-12 Thread Alexander Bluhm 
On Mon, Jan 12, 2015 at 03:00:41PM +0100, Martin Pieuchot wrote:
 @@ -1138,7 +1140,8 @@ rt_ifa_add(struct ifaddr *ifa, int flags
* userland that a new address has been added.
*/
   if (flags  RTF_LOCAL)
 - rt_newaddrmsg(RTM_ADD, ifa, error, nrt);
 + rt_sendaddrmsg(nrt, RTM_NEWADDR);
 + rt_sendmsg(nrt, RTM_ADD, rtableid);
   }
   return (error);
  }
 @@ -1193,7 +1196,8 @@ rt_ifa_del(struct ifaddr *ifa, int flags
   error = rtrequest1(RTM_DELETE, info, prio, nrt, rtableid);
   if (error == 0  (rt = nrt) != NULL) {
   if (flags  RTF_LOCAL)
 - rt_newaddrmsg(RTM_DELETE, ifa, error, nrt);
 + rt_sendaddrmsg(nrt, RTM_DELADDR);
 + rt_sendmsg(nrt, RTM_DELETE, rtableid);
   if (rt-rt_refcnt = 0) {
   rt-rt_refcnt++;
   rtfree(rt);

The purpose of the pass 1,2 loop was to reverse the order of the
address and route message in the delete case.  Although I don't
know wether the order is important to anybody, I would not change
the behavior.

Can we change the second chunk to this?

rt_sendmsg(nrt, RTM_DELETE, rtableid);
if (flags  RTF_LOCAL)
rt_sendaddrmsg(nrt, RTM_DELADDR);

otherwise OK bluhm@

Re: Sending route messages for local routes or cloning routes

2015-01-07 Thread Florian Riehm 
Hi Martin,

Thanks for your diff! Regardless of my problem it makes our code
more clear. The loop in rt_newaddrmsg() was ugly.

 
 Here's a diff that should generate a RTM_ADD message for every CLONING
 route added while keeping the existing RTM_NEWADDR/RTM_DELADDR logic.
 
 dhclient(8) is happy with this change, does it fix your use case too?

There is a small bug in your diff of route.c, because rt_ifa_del() is 
never called with flag RTF_CLONING, so ospfd doesn't notice if an address
gets deleted.

I was thinking about this three options to fix the problem:
1.) Check with rt-rt_flags instead of flags for RTF_CLONING flag:
-if (flags  (RTF_LOCAL|RTF_CLONING))
+if (rt-rt_flags  (RTF_LOCAL|RTF_CLONING))

2) Add RTF_CLONING to flags argument at certain calls of rt_ifa_add() /
rt_ifa_delete()

3.) Remove the check completely and call always rt_sendmsg()

At the moment I would prefer the 3. solution. After rtrequest1() has been
called, we check for error and check if rt ist not NULL. If this conditions are
true we have added or deleted a route. Why should we not send a route message
then? 
Below an updated diff for route.c with my fix.

I noticed that the RTM_ADD/RTM_DELETE routing messages doesn't contain a 
priority anymore with your diff. I guess this is not a problem.
If nobody needs the priority I prefer the new behavior.
If something needs it, we had to add another argument to rt_missmsg().

Regards

Florian

Index: net/route.c
===
RCS file: /cvs/src/sys/net/route.c,v
retrieving revision 1.196
diff -u -p -r1.196 route.c
--- net/route.c 29 Dec 2014 11:53:58 -  1.196
+++ net/route.c 7 Jan 2015 17:29:56 -
@@ -382,11 +382,13 @@ void
 rt_sendmsg(struct rtentry *rt, int cmd, u_int rtableid)
 {
struct rt_addrinfo info;
+   struct sockaddr_rtlabel sa_rl;
 
-   bzero(info, sizeof(info));
+   memset(info, 0, sizeof(info));
info.rti_info[RTAX_DST] = rt_key(rt);
info.rti_info[RTAX_GATEWAY] = rt-rt_gateway;
info.rti_info[RTAX_NETMASK] = rt_mask(rt);
+   info.rti_info[RTAX_LABEL] = rtlabel_id2sa(rt-rt_labelid, sa_rl);
if (rt-rt_ifp != NULL) {
info.rti_info[RTAX_IFP] =(struct sockaddr *)rt-rt_ifp-if_sadl;
info.rti_info[RTAX_IFA] = rt-rt_ifa-ifa_addr;
@@ -1098,7 +1100,8 @@ rt_ifa_add(struct ifaddr *ifa, int flags
 * userland that a new address has been added.
 */
if (flags  RTF_LOCAL)
-   rt_newaddrmsg(RTM_ADD, ifa, error, nrt);
+   rt_sendaddrmsg(nrt, RTM_NEWADDR);
+   rt_sendmsg(nrt, RTM_ADD, rtableid);
}
return (error);
 }
@@ -1153,7 +1156,8 @@ rt_ifa_del(struct ifaddr *ifa, int flags
error = rtrequest1(RTM_DELETE, info, prio, nrt, rtableid);
if (error == 0  (rt = nrt) != NULL) {
if (flags  RTF_LOCAL)
-   rt_newaddrmsg(RTM_DELETE, ifa, error, nrt);
+   rt_sendaddrmsg(nrt, RTM_DELADDR);
+   rt_sendmsg(nrt, RTM_DELETE, rtableid);
if (rt-rt_refcnt = 0) {
rt-rt_refcnt++;
rtfree(rt);

Re: Sending route messages for local routes or cloning routes

2015-01-06 Thread Martin Pieuchot 
On 23/12/14(Tue) 14:43, Florian Riehm wrote:
 On 12/23/14 11:59, Martin Pieuchot wrote:
  Would it make sense to remove the loop in rt_newaddrmsg which generates 
  the two
   route messages? Instead of this rt_newaddrmsg sends only the RTM_NEWADDR 
   /
   RTM_DELADDR message and the other message gets send after 
   creating/deleting the
   cloning route.
  I think it does make sense.  It would restore the RTM_ADD for
  RTF_CLONING routes and keep one RTM_NEWADDR for RTF_LOCAL routes.
  Apart from your scenario with ospfd/ospf6d, dhclient should be happy
  with this change and I can think of a third case.  If  you configure
  two addresses of the same subnet you should see 2 RTM_NEWADDR but only
  one RTM_ADD since only the first address will get a cloning route.
  
   By the way if rt_newaddrmsg() sends RTM_NEWADDR and RTM_DELADDR we should
   rename it to rt_addrmsg().
  If you remove the loop and generate only one message, I think that you can
  simply use rt_sendmsg() and kill rt_newaddrmsg().
 
 
 ok, thanks for your advice. I will try it and let you know if it works.

Here's a diff that should generate a RTM_ADD message for every CLONING
route added while keeping the existing RTM_NEWADDR/RTM_DELADDR logic.

dhclient(8) is happy with this change, does it fix your use case too?

Index: net/route.c
===
RCS file: /home/ncvs/src/sys/net/route.c,v
retrieving revision 1.196
diff -u -p -r1.196 route.c
--- net/route.c 29 Dec 2014 11:53:58 -  1.196
+++ net/route.c 6 Jan 2015 12:15:04 -
@@ -382,11 +382,13 @@ void
 rt_sendmsg(struct rtentry *rt, int cmd, u_int rtableid)
 {
struct rt_addrinfo info;
+   struct sockaddr_rtlabel sa_rl;
 
-   bzero(info, sizeof(info));
+   memset(info, 0, sizeof(info));
info.rti_info[RTAX_DST] = rt_key(rt);
info.rti_info[RTAX_GATEWAY] = rt-rt_gateway;
info.rti_info[RTAX_NETMASK] = rt_mask(rt);
+   info.rti_info[RTAX_LABEL] = rtlabel_id2sa(rt-rt_labelid, sa_rl);
if (rt-rt_ifp != NULL) {
info.rti_info[RTAX_IFP] =(struct sockaddr *)rt-rt_ifp-if_sadl;
info.rti_info[RTAX_IFA] = rt-rt_ifa-ifa_addr;
@@ -1098,7 +1100,9 @@ rt_ifa_add(struct ifaddr *ifa, int flags
 * userland that a new address has been added.
 */
if (flags  RTF_LOCAL)
-   rt_newaddrmsg(RTM_ADD, ifa, error, nrt);
+   rt_sendaddrmsg(nrt, RTM_NEWADDR);
+   if (flags  (RTF_LOCAL|RTF_CLONING))
+   rt_sendmsg(nrt, RTM_ADD, rtableid);
}
return (error);
 }
@@ -1153,7 +1157,9 @@ rt_ifa_del(struct ifaddr *ifa, int flags
error = rtrequest1(RTM_DELETE, info, prio, nrt, rtableid);
if (error == 0  (rt = nrt) != NULL) {
if (flags  RTF_LOCAL)
-   rt_newaddrmsg(RTM_DELETE, ifa, error, nrt);
+   rt_sendaddrmsg(nrt, RTM_DELADDR);
+   if (flags  (RTF_LOCAL|RTF_CLONING))
+   rt_sendmsg(nrt, RTM_DELETE, rtableid);
if (rt-rt_refcnt = 0) {
rt-rt_refcnt++;
rtfree(rt);
Index: net/route.h
===
RCS file: /home/ncvs/src/sys/net/route.h,v
retrieving revision 1.101
diff -u -p -r1.101 route.h
--- net/route.h 24 Nov 2014 12:43:54 -  1.101
+++ net/route.h 6 Jan 2015 12:15:04 -
@@ -355,9 +355,9 @@ void rt_ifannouncemsg(struct ifnet *, i
 voidrt_maskedcopy(struct sockaddr *,
struct sockaddr *, struct sockaddr *);
 voidrt_sendmsg(struct rtentry *, int, u_int);
+voidrt_sendaddrmsg(struct rtentry *, int);
 voidrt_missmsg(int, struct rt_addrinfo *, int, struct ifnet *, int,
u_int);
-voidrt_newaddrmsg(int, struct ifaddr *, int, struct rtentry *);
 int rt_setgate(struct rtentry *, struct sockaddr *,
struct sockaddr *, u_int);
 voidrt_setmetrics(u_long, struct rt_metrics *, struct rt_kmetrics *);
Index: net/rtsock.c
===
RCS file: /home/ncvs/src/sys/net/rtsock.c,v
retrieving revision 1.155
diff -u -p -r1.155 rtsock.c
--- net/rtsock.c19 Dec 2014 18:57:17 -  1.155
+++ net/rtsock.c6 Jan 2015 12:15:04 -
@@ -1137,70 +1137,36 @@ rt_ifmsg(struct ifnet *ifp)
  * copies of it.
  */
 void
-rt_newaddrmsg(int cmd, struct ifaddr *ifa, int error, struct rtentry *rt)
+rt_sendaddrmsg(struct rtentry *rt, int cmd)
 {
-   struct rt_addrinfo   info;
-   struct sockaddr *sa = NULL;
-   int  pass;
-   struct mbuf *m = NULL;
+   struct ifaddr   *ifa = rt-rt_ifa;
struct ifnet*ifp = ifa-ifa_ifp;
+   struct mbuf *m = NULL;
+   struct rt_addrinfo   info;
+   struct ifa_msghdr

Sending route messages for local routes or cloning routes

2014-12-23 Thread Florian Riehm 
Hi Martin,

as requested in your commit message I would like to tell you about a regression
with the introduced local routes:

Before OpenBSD 5.6 it was possible to add route labels to interfaces and tell
ospfd to redistribute all labeled routes. After adding an address to a labeled
interface a new route was announced via ospf.

Now the ospfd can't detect added or deleted addresses anymore, because the
kernel informs about local routes instead of cloning routes.

route -nv monitor:

OPENBSD_5_5:
RTM_NEWADDR: address being added to iface: len 96, metric 0, flags:CLONING
sockaddrs: NETMASK,IFP,IFA,BRD
 255.255.255.0 08:00:27:ad:dd:7c 5.5.5.5 5.5.5.255

RTM_ADD: Add Route: len 192, priority 4, table 0, pid: 0, seq 0, errno 0
flags:UP,CLONING
use:0   mtu:0expire:0 
locks:  inits: 
sockaddrs: DST,GATEWAY,NETMASK,LABEL
 5.5.5.0 link#3 255.255.255.0 EXPORT

OPENBSD_5_6:
RTM_NEWADDR: address being added to iface: len 96, metric 0, flags:UP
sockaddrs: NETMASK,IFP,IFA,BRD
 255.255.255.0 08:00:27:fd:b8:81 5.5.5.5 5.5.5.255

RTM_ADD: Add Route: len 184, priority 1, table 0, pid: 0, seq 0, errno 0
flags:UP,HOST,LLINFO,LOCAL
use:0   mtu:0expire:0 
locks:  inits: 
sockaddrs: DST,GATEWAY,LABEL
 5.5.5.5 08:00:27:fd:b8:81 EXPORT

In the commit message of route.c r1.172 you said always generating one message
per locally configured address.
Are any userland tools needing this message?

Would it make sense to remove the loop in rt_newaddrmsg which generates the two
route messages? Instead of this rt_newaddrmsg sends only the RTM_NEWADDR /
RTM_DELADDR message and the other message gets send after creating/deleting the
cloning route.

By the way if rt_newaddrmsg() sends RTM_NEWADDR and RTM_DELADDR we should
rename it to rt_addrmsg().

Regards,

Florian

Re: Sending route messages for local routes or cloning routes

2014-12-23 Thread Martin Pieuchot 
Hello Florian,

On 23/12/14(Tue) 11:17, Florian Riehm wrote:
 Hi Martin,
 
 as requested in your commit message I would like to tell you about a 
 regression
 with the introduced local routes:
 
 Before OpenBSD 5.6 it was possible to add route labels to interfaces and tell
 ospfd to redistribute all labeled routes. After adding an address to a labeled
 interface a new route was announced via ospf.
 
 Now the ospfd can't detect added or deleted addresses anymore, because the
 kernel informs about local routes instead of cloning routes.

Thanks for your attention and the report!

 route -nv monitor:
 
 OPENBSD_5_5:
 RTM_NEWADDR: address being added to iface: len 96, metric 0, flags:CLONING
 sockaddrs: NETMASK,IFP,IFA,BRD
  255.255.255.0 08:00:27:ad:dd:7c 5.5.5.5 5.5.5.255
 
 RTM_ADD: Add Route: len 192, priority 4, table 0, pid: 0, seq 0, errno 0
 flags:UP,CLONING
 use:0   mtu:0expire:0 
 locks:  inits: 
 sockaddrs: DST,GATEWAY,NETMASK,LABEL
  5.5.5.0 link#3 255.255.255.0 EXPORT
 
 OPENBSD_5_6:
 RTM_NEWADDR: address being added to iface: len 96, metric 0, flags:UP
 sockaddrs: NETMASK,IFP,IFA,BRD
  255.255.255.0 08:00:27:fd:b8:81 5.5.5.5 5.5.5.255
 
 RTM_ADD: Add Route: len 184, priority 1, table 0, pid: 0, seq 0, errno 0
 flags:UP,HOST,LLINFO,LOCAL
 use:0   mtu:0expire:0 
 locks:  inits: 
 sockaddrs: DST,GATEWAY,LABEL
  5.5.5.5 08:00:27:fd:b8:81 EXPORT
 
 In the commit message of route.c r1.172 you said always generating one 
 message
 per locally configured address.
 Are any userland tools needing this message?

At least dhclient(8).

 Would it make sense to remove the loop in rt_newaddrmsg which generates the 
 two
 route messages? Instead of this rt_newaddrmsg sends only the RTM_NEWADDR /
 RTM_DELADDR message and the other message gets send after creating/deleting 
 the
 cloning route.

I think it does make sense.  It would restore the RTM_ADD for
RTF_CLONING routes and keep one RTM_NEWADDR for RTF_LOCAL routes.
Apart from your scenario with ospfd/ospf6d, dhclient should be happy
with this change and I can think of a third case.  If  you configure
two addresses of the same subnet you should see 2 RTM_NEWADDR but only
one RTM_ADD since only the first address will get a cloning route.

 By the way if rt_newaddrmsg() sends RTM_NEWADDR and RTM_DELADDR we should
 rename it to rt_addrmsg().

If you remove the loop and generate only one message, I think that you can
simply use rt_sendmsg() and kill rt_newaddrmsg().

Re: Sending route messages for local routes or cloning routes

2014-12-23 Thread Florian Riehm 
On 12/23/14 11:59, Martin Pieuchot wrote:
 Would it make sense to remove the loop in rt_newaddrmsg which generates the 
 two
  route messages? Instead of this rt_newaddrmsg sends only the RTM_NEWADDR /
  RTM_DELADDR message and the other message gets send after 
  creating/deleting the
  cloning route.
 I think it does make sense.  It would restore the RTM_ADD for
 RTF_CLONING routes and keep one RTM_NEWADDR for RTF_LOCAL routes.
 Apart from your scenario with ospfd/ospf6d, dhclient should be happy
 with this change and I can think of a third case.  If  you configure
 two addresses of the same subnet you should see 2 RTM_NEWADDR but only
 one RTM_ADD since only the first address will get a cloning route.
 
  By the way if rt_newaddrmsg() sends RTM_NEWADDR and RTM_DELADDR we should
  rename it to rt_addrmsg().
 If you remove the loop and generate only one message, I think that you can
 simply use rt_sendmsg() and kill rt_newaddrmsg().


ok, thanks for your advice. I will try it and let you know if it works.