Re: Remove rti_ifp from struct rt_addrinfo
On 25/04/14(Fri) 21:05, Henning Brauer wrote:
* Ryan McBride mcbr...@openbsd.org [2014-04-25 10:31]:
On Fri, Apr 25, 2014 at 02:40:57AM +0200, Alexander Bluhm wrote:
On Fri, Apr 25, 2014 at 09:09:03AM +0900, Ryan McBride wrote:
Part of the reason it's there is to make carp work properly for services
listening on the carp interface, in particular so that hosts in the
BACKUP state will reach the MASTER rather than trying and failing to
connect to their own carp interface. Maybe not needed in all setups, but
likely to break things if we simply remove it.
Why do you want to connect from the BACKUP machine to the MASTER
using CARP addresses? Just add another fixed address and you can
do that.
Two reasons that come to mind are:
1) For troubleshooting, so I can ping or otherwise monitor the MASTER
host.
2) In some cases it's undisireable (or even not possible) to run
services on other IP addresses. For example, services that only allow
you to configure 1 listening IP, or services where you wish to avoid
users connecting to anything but the MASTER server.
Well maybe it has been done for good reasons... but the fact is that it
does not work anymore. I don't know for how long it has been broken,
I just tried with a -current kernel and it does not work.
The current implementation may change the routing table in subtile
ways until nothing works. In IPv6 the routes are fixed and there
are less problems.
In my opinion the current (intended) behaviour is correct; my preference
would be to see this fixed rather than removed.
Well this (intended) behavior does not work. How can it be correct?
And since nobody reported a bug about this (intended) behaviour, I
believe nobody cares. So, here's a diff to kill this function (:
given that
-it is done for v4 only
-it has been demonstrated to cause problems, namely screwed up routing
tables
-it, afair, not working in the unnumbered case at all
the only conclusion I can come to is nuke it!. especially due to the
2nd point. I causes more harm than good in its current state.
if this is desired (I can't really see the need to be honest) it must
be done properly doing route priorities and marking routes down. This
functionaity didn't exist when we did carp. Going that route (haha),
the code for that wouldn't have much in common with what is currently
there, so... I'm in favor of nuking.
Indeed, let's kill it, if somebody wants this feature, he or she will
implement it in a proper way.
Ok?
Index: netinet/ip_carp.c
===
RCS file: /home/ncvs/src/sys/netinet/ip_carp.c,v
retrieving revision 1.228
diff -u -p -r1.228 ip_carp.c
--- netinet/ip_carp.c 21 Apr 2014 12:22:26 - 1.228
+++ netinet/ip_carp.c 28 Apr 2014 12:59:17 -
@@ -198,7 +198,6 @@ voidcarp_hmac_generate(struct carp_vhos
unsigned char *, u_int8_t);
intcarp_hmac_verify(struct carp_vhost_entry *, u_int32_t *,
unsigned char *);
-void carp_setroute(struct carp_softc *, int);
void carp_proto_input_c(struct mbuf *, struct carp_header *, int,
sa_family_t);
void carpattach(int);
@@ -396,124 +395,6 @@ carp_hmac_verify(struct carp_vhost_entry
return (1);
}
-void
-carp_setroute(struct carp_softc *sc, int cmd)
-{
- struct ifaddr *ifa;
- int s;
-
- /* XXX this mess needs fixing */
-
- s = splsoftnet();
- TAILQ_FOREACH(ifa, sc-sc_if.if_addrlist, ifa_list) {
- switch (ifa-ifa_addr-sa_family) {
- case AF_INET: {
- int error;
- struct sockaddr sa;
- struct rtentry *rt;
- struct radix_node_head *rnh;
- struct radix_node *rn;
- struct rt_addrinfo info;
- int hr_otherif, nr_ourif;
- struct sockaddr_rtlabel sa_rl;
- const char *label;
-
- /* Remove the existing host route, if any */
- memset(info, 0, sizeof(info));
- info.rti_info[RTAX_DST] = ifa-ifa_addr;
- info.rti_flags = RTF_HOST;
- error = rtrequest1(RTM_DELETE, info, RTP_CONNECTED,
- NULL, sc-sc_if.if_rdomain);
- rt_missmsg(RTM_DELETE, info, info.rti_flags, NULL,
- error, sc-sc_if.if_rdomain);
-
- /* Check for our address on another interface */
- /* XXX cries for proper API */
- rnh = rtable_get(sc-sc_if.if_rdomain,
- ifa-ifa_addr-sa_family);
- rn = rnh-rnh_matchaddr(ifa-ifa_addr, rnh);
- rt = (struct rtentry *)rn;
- hr_otherif = (rt rt-rt_ifp != sc-sc_if
Re: Remove rti_ifp from struct rt_addrinfo
On Fri, Apr 25, 2014 at 02:40:57AM +0200, Alexander Bluhm wrote: On Fri, Apr 25, 2014 at 09:09:03AM +0900, Ryan McBride wrote: Part of the reason it's there is to make carp work properly for services listening on the carp interface, in particular so that hosts in the BACKUP state will reach the MASTER rather than trying and failing to connect to their own carp interface. Maybe not needed in all setups, but likely to break things if we simply remove it. Why do you want to connect from the BACKUP machine to the MASTER using CARP addresses? Just add another fixed address and you can do that. Two reasons that come to mind are: 1) For troubleshooting, so I can ping or otherwise monitor the MASTER host. 2) In some cases it's undisireable (or even not possible) to run services on other IP addresses. For example, services that only allow you to configure 1 listening IP, or services where you wish to avoid users connecting to anything but the MASTER server. The current implementation may change the routing table in subtile ways until nothing works. In IPv6 the routes are fixed and there are less problems. In my opinion the current (intended) behaviour is correct; my preference would be to see this fixed rather than removed.
Re: Remove rti_ifp from struct rt_addrinfo
* Ryan McBride mcbr...@openbsd.org [2014-04-25 10:31]: On Fri, Apr 25, 2014 at 02:40:57AM +0200, Alexander Bluhm wrote: On Fri, Apr 25, 2014 at 09:09:03AM +0900, Ryan McBride wrote: Part of the reason it's there is to make carp work properly for services listening on the carp interface, in particular so that hosts in the BACKUP state will reach the MASTER rather than trying and failing to connect to their own carp interface. Maybe not needed in all setups, but likely to break things if we simply remove it. Why do you want to connect from the BACKUP machine to the MASTER using CARP addresses? Just add another fixed address and you can do that. Two reasons that come to mind are: 1) For troubleshooting, so I can ping or otherwise monitor the MASTER host. 2) In some cases it's undisireable (or even not possible) to run services on other IP addresses. For example, services that only allow you to configure 1 listening IP, or services where you wish to avoid users connecting to anything but the MASTER server. The current implementation may change the routing table in subtile ways until nothing works. In IPv6 the routes are fixed and there are less problems. In my opinion the current (intended) behaviour is correct; my preference would be to see this fixed rather than removed. given that -it is done for v4 only -it has been demonstrated to cause problems, namely screwed up routing tables -it, afair, not working in the unnumbered case at all the only conclusion I can come to is nuke it!. especially due to the 2nd point. I causes more harm than good in its current state. if this is desired (I can't really see the need to be honest) it must be done properly doing route priorities and marking routes down. This functionaity didn't exist when we did carp. Going that route (haha), the code for that wouldn't have much in common with what is currently there, so... I'm in favor of nuking. coincidently, I have a diff which does that :) -- Henning Brauer, h...@bsws.de, henn...@openbsd.org BS Web Services GmbH, http://bsws.de, Full-Service ISP Secure Hosting, Mail and DNS Services. Dedicated Servers, Root to Fully Managed Henning Brauer Consulting, http://henningbrauer.com/
Remove rti_ifp from struct rt_addrinfo
This ifp pointer is only needed by rt_getifa() to find an address, so
make it a local variable.
The rtrequest1(9) change might introduce a negligible slowdown since
I remove the already known ifp pointer. But this only happens in the
case described in the comment just before and I would bet because of
carp_setroute(), still nobody to fix this? It's not better than
OpenSSL...
In the rtsock chunk, the two pointers are equivalent.
Ok?
Index: net/route.c
===
RCS file: /home/ncvs/src/sys/net/route.c,v
retrieving revision 1.163
diff -u -p -r1.163 route.c
--- net/route.c 23 Apr 2014 09:30:57 - 1.163
+++ net/route.c 24 Apr 2014 11:03:35 -
@@ -691,16 +691,17 @@ int
rt_getifa(struct rt_addrinfo *info, u_int rtid)
{
struct ifaddr *ifa;
+ struct ifnet*ifp;
/*
* ifp may be specified by sockaddr_dl when protocol address
* is ambiguous
*/
- if (info-rti_ifp == NULL info-rti_info[RTAX_IFP] != NULL) {
+ if (info-rti_info[RTAX_IFP] != NULL) {
struct sockaddr_dl *sdl;
sdl = (struct sockaddr_dl *)info-rti_info[RTAX_IFP];
- info-rti_ifp = if_get(sdl-sdl_index);
+ ifp = if_get(sdl-sdl_index);
}
if (info-rti_ifa == NULL info-rti_info[RTAX_IFA] != NULL)
@@ -713,8 +714,8 @@ rt_getifa(struct rt_addrinfo *info, u_in
if ((sa = info-rti_info[RTAX_GATEWAY]) == NULL)
sa = info-rti_info[RTAX_DST];
- if (sa != NULL info-rti_ifp != NULL)
- info-rti_ifa = ifaof_ifpforaddr(sa, info-rti_ifp);
+ if (sa != NULL ifp != NULL)
+ info-rti_ifa = ifaof_ifpforaddr(sa, ifp);
else if (info-rti_info[RTAX_DST] != NULL
info-rti_info[RTAX_GATEWAY] != NULL)
info-rti_ifa = ifa_ifwithroute(info-rti_flags,
@@ -729,9 +730,6 @@ rt_getifa(struct rt_addrinfo *info, u_in
if ((ifa = info-rti_ifa) == NULL)
return (ENETUNREACH);
- if (info-rti_ifp == NULL)
- info-rti_ifp = ifa-ifa_ifp;
-
return (0);
}
@@ -828,8 +826,10 @@ rtrequest1(int req, struct rt_addrinfo *
info-rti_ifa = rt-rt_ifa;
} else {
/*
-* The interface address at the cloning route
-* is not longer referenced by an interface.
+* The address of the cloning route is not longer
+* configured on an interface, but its descriptor
+* is still there because of reference counting.
+*
* Try to find a similar active address and use
* it for the cloned route. The cloning route
* will get the new address and interface later.
@@ -837,7 +837,6 @@ rtrequest1(int req, struct rt_addrinfo *
info-rti_ifa = NULL;
info-rti_info[RTAX_IFA] = rt-rt_ifa-ifa_addr;
}
- info-rti_ifp = rt-rt_ifp;
info-rti_flags = rt-rt_flags ~(RTF_CLONING | RTF_STATIC);
info-rti_flags |= RTF_CLONED;
info-rti_info[RTAX_GATEWAY] = rt-rt_gateway;
Index: net/route.h
===
RCS file: /home/ncvs/src/sys/net/route.h,v
retrieving revision 1.91
diff -u -p -r1.91 route.h
--- net/route.h 10 Apr 2014 13:47:21 - 1.91
+++ net/route.h 24 Apr 2014 11:03:35 -
@@ -299,7 +299,6 @@ struct rt_addrinfo {
struct sockaddr *rti_info[RTAX_MAX];
int rti_flags;
struct ifaddr *rti_ifa;
- struct ifnet *rti_ifp;
struct rt_msghdr *rti_rtm;
u_char rti_mpls;
};
Index: net/rtsock.c
===
RCS file: /home/ncvs/src/sys/net/rtsock.c,v
retrieving revision 1.142
diff -u -p -r1.142 rtsock.c
--- net/rtsock.c18 Mar 2014 10:47:34 - 1.142
+++ net/rtsock.c24 Apr 2014 11:03:35 -
@@ -768,7 +768,7 @@ report:
ifafree(rt-rt_ifa);
rt-rt_ifa = ifa;
ifa-ifa_refcnt++;
- rt-rt_ifp = info.rti_ifp;
+ rt-rt_ifp = ifa-ifa_ifp;
#ifndef SMALL_KERNEL
/* recheck link state after ifp change*/
rt_if_linkstate_change(
Re: Remove rti_ifp from struct rt_addrinfo
* Martin Pieuchot mpieuc...@nolizard.org [2014-04-24 13:24]: This ifp pointer is only needed by rt_getifa() to find an address, so make it a local variable. The rtrequest1(9) change might introduce a negligible slowdown since I remove the already known ifp pointer. But this only happens in the case described in the comment just before and I would bet because of carp_setroute(), still nobody to fix this? It's not better than OpenSSL... In the rtsock chunk, the two pointers are equivalent. Ok? yup. the carp route fiddling is pretty damn mad, and with the route priorities and the ability to mark routes as down there should be a much cleaner way to do this these days. heck, the entire carp route fiddling needs to be reassesed. things changed, i can\t even fully remeber why it is there (i think it was about backup nodes still being able to reach a network only present on the carp if or the like), and i seem to remember it doesn't quite work as expected anyway, but don't take my word for it, memory REALLY fuzzy on that front. -- Henning Brauer, h...@bsws.de, henn...@openbsd.org BS Web Services GmbH, http://bsws.de, Full-Service ISP Secure Hosting, Mail and DNS Services. Dedicated Servers, Root to Fully Managed Henning Brauer Consulting, http://henningbrauer.com/
Re: Remove rti_ifp from struct rt_addrinfo
On 24/04/14(Thu) 13:43, Henning Brauer wrote:
* Martin Pieuchot mpieuc...@nolizard.org [2014-04-24 13:24]:
This ifp pointer is only needed by rt_getifa() to find an address, so
make it a local variable.
The rtrequest1(9) change might introduce a negligible slowdown since
I remove the already known ifp pointer. But this only happens in the
case described in the comment just before and I would bet because of
carp_setroute(), still nobody to fix this? It's not better than
OpenSSL...
In the rtsock chunk, the two pointers are equivalent.
Ok?
yup.
And now with proper ifp initialization, pointed by bluhm@.
Index: net/route.c
===
RCS file: /home/ncvs/src/sys/net/route.c,v
retrieving revision 1.163
diff -u -p -r1.163 route.c
--- net/route.c 23 Apr 2014 09:30:57 - 1.163
+++ net/route.c 24 Apr 2014 12:45:04 -
@@ -691,16 +691,17 @@ int
rt_getifa(struct rt_addrinfo *info, u_int rtid)
{
struct ifaddr *ifa;
+ struct ifnet*ifp = NULL;
/*
* ifp may be specified by sockaddr_dl when protocol address
* is ambiguous
*/
- if (info-rti_ifp == NULL info-rti_info[RTAX_IFP] != NULL) {
+ if (info-rti_info[RTAX_IFP] != NULL) {
struct sockaddr_dl *sdl;
sdl = (struct sockaddr_dl *)info-rti_info[RTAX_IFP];
- info-rti_ifp = if_get(sdl-sdl_index);
+ ifp = if_get(sdl-sdl_index);
}
if (info-rti_ifa == NULL info-rti_info[RTAX_IFA] != NULL)
@@ -713,8 +714,8 @@ rt_getifa(struct rt_addrinfo *info, u_in
if ((sa = info-rti_info[RTAX_GATEWAY]) == NULL)
sa = info-rti_info[RTAX_DST];
- if (sa != NULL info-rti_ifp != NULL)
- info-rti_ifa = ifaof_ifpforaddr(sa, info-rti_ifp);
+ if (sa != NULL ifp != NULL)
+ info-rti_ifa = ifaof_ifpforaddr(sa, ifp);
else if (info-rti_info[RTAX_DST] != NULL
info-rti_info[RTAX_GATEWAY] != NULL)
info-rti_ifa = ifa_ifwithroute(info-rti_flags,
@@ -729,9 +730,6 @@ rt_getifa(struct rt_addrinfo *info, u_in
if ((ifa = info-rti_ifa) == NULL)
return (ENETUNREACH);
- if (info-rti_ifp == NULL)
- info-rti_ifp = ifa-ifa_ifp;
-
return (0);
}
@@ -828,8 +826,10 @@ rtrequest1(int req, struct rt_addrinfo *
info-rti_ifa = rt-rt_ifa;
} else {
/*
-* The interface address at the cloning route
-* is not longer referenced by an interface.
+* The address of the cloning route is not longer
+* configured on an interface, but its descriptor
+* is still there because of reference counting.
+*
* Try to find a similar active address and use
* it for the cloned route. The cloning route
* will get the new address and interface later.
@@ -837,7 +837,6 @@ rtrequest1(int req, struct rt_addrinfo *
info-rti_ifa = NULL;
info-rti_info[RTAX_IFA] = rt-rt_ifa-ifa_addr;
}
- info-rti_ifp = rt-rt_ifp;
info-rti_flags = rt-rt_flags ~(RTF_CLONING | RTF_STATIC);
info-rti_flags |= RTF_CLONED;
info-rti_info[RTAX_GATEWAY] = rt-rt_gateway;
Index: net/route.h
===
RCS file: /home/ncvs/src/sys/net/route.h,v
retrieving revision 1.91
diff -u -p -r1.91 route.h
--- net/route.h 10 Apr 2014 13:47:21 - 1.91
+++ net/route.h 24 Apr 2014 12:45:04 -
@@ -299,7 +299,6 @@ struct rt_addrinfo {
struct sockaddr *rti_info[RTAX_MAX];
int rti_flags;
struct ifaddr *rti_ifa;
- struct ifnet *rti_ifp;
struct rt_msghdr *rti_rtm;
u_char rti_mpls;
};
Index: net/rtsock.c
===
RCS file: /home/ncvs/src/sys/net/rtsock.c,v
retrieving revision 1.142
diff -u -p -r1.142 rtsock.c
--- net/rtsock.c18 Mar 2014 10:47:34 - 1.142
+++ net/rtsock.c24 Apr 2014 12:45:04 -
@@ -768,7 +768,7 @@ report:
ifafree(rt-rt_ifa);
rt-rt_ifa = ifa;
ifa-ifa_refcnt++;
- rt-rt_ifp = info.rti_ifp;
+ rt-rt_ifp = ifa-ifa_ifp;
#ifndef SMALL_KERNEL
/* recheck link state after ifp change*/
rt_if_linkstate_change(
Re: Remove rti_ifp from struct rt_addrinfo
On Thu, Apr 24, 2014 at 02:52:27PM +0200, Martin Pieuchot wrote:
On 24/04/14(Thu) 13:43, Henning Brauer wrote:
* Martin Pieuchot mpieuc...@nolizard.org [2014-04-24 13:24]:
This ifp pointer is only needed by rt_getifa() to find an address, so
make it a local variable.
The rtrequest1(9) change might introduce a negligible slowdown since
I remove the already known ifp pointer. But this only happens in the
case described in the comment just before and I would bet because of
carp_setroute(), still nobody to fix this? It's not better than
OpenSSL...
In the rtsock chunk, the two pointers are equivalent.
Ok?
yup.
And now with proper ifp initialization, pointed by bluhm@.
OK bluhm@
Index: net/route.c
===
RCS file: /home/ncvs/src/sys/net/route.c,v
retrieving revision 1.163
diff -u -p -r1.163 route.c
--- net/route.c 23 Apr 2014 09:30:57 - 1.163
+++ net/route.c 24 Apr 2014 12:45:04 -
@@ -691,16 +691,17 @@ int
rt_getifa(struct rt_addrinfo *info, u_int rtid)
{
struct ifaddr *ifa;
+ struct ifnet*ifp = NULL;
/*
* ifp may be specified by sockaddr_dl when protocol address
* is ambiguous
*/
- if (info-rti_ifp == NULL info-rti_info[RTAX_IFP] != NULL) {
+ if (info-rti_info[RTAX_IFP] != NULL) {
struct sockaddr_dl *sdl;
sdl = (struct sockaddr_dl *)info-rti_info[RTAX_IFP];
- info-rti_ifp = if_get(sdl-sdl_index);
+ ifp = if_get(sdl-sdl_index);
}
if (info-rti_ifa == NULL info-rti_info[RTAX_IFA] != NULL)
@@ -713,8 +714,8 @@ rt_getifa(struct rt_addrinfo *info, u_in
if ((sa = info-rti_info[RTAX_GATEWAY]) == NULL)
sa = info-rti_info[RTAX_DST];
- if (sa != NULL info-rti_ifp != NULL)
- info-rti_ifa = ifaof_ifpforaddr(sa, info-rti_ifp);
+ if (sa != NULL ifp != NULL)
+ info-rti_ifa = ifaof_ifpforaddr(sa, ifp);
else if (info-rti_info[RTAX_DST] != NULL
info-rti_info[RTAX_GATEWAY] != NULL)
info-rti_ifa = ifa_ifwithroute(info-rti_flags,
@@ -729,9 +730,6 @@ rt_getifa(struct rt_addrinfo *info, u_in
if ((ifa = info-rti_ifa) == NULL)
return (ENETUNREACH);
- if (info-rti_ifp == NULL)
- info-rti_ifp = ifa-ifa_ifp;
-
return (0);
}
@@ -828,8 +826,10 @@ rtrequest1(int req, struct rt_addrinfo *
info-rti_ifa = rt-rt_ifa;
} else {
/*
- * The interface address at the cloning route
- * is not longer referenced by an interface.
+ * The address of the cloning route is not longer
+ * configured on an interface, but its descriptor
+ * is still there because of reference counting.
+ *
* Try to find a similar active address and use
* it for the cloned route. The cloning route
* will get the new address and interface later.
@@ -837,7 +837,6 @@ rtrequest1(int req, struct rt_addrinfo *
info-rti_ifa = NULL;
info-rti_info[RTAX_IFA] = rt-rt_ifa-ifa_addr;
}
- info-rti_ifp = rt-rt_ifp;
info-rti_flags = rt-rt_flags ~(RTF_CLONING | RTF_STATIC);
info-rti_flags |= RTF_CLONED;
info-rti_info[RTAX_GATEWAY] = rt-rt_gateway;
Index: net/route.h
===
RCS file: /home/ncvs/src/sys/net/route.h,v
retrieving revision 1.91
diff -u -p -r1.91 route.h
--- net/route.h 10 Apr 2014 13:47:21 - 1.91
+++ net/route.h 24 Apr 2014 12:45:04 -
@@ -299,7 +299,6 @@ struct rt_addrinfo {
struct sockaddr *rti_info[RTAX_MAX];
int rti_flags;
struct ifaddr *rti_ifa;
- struct ifnet *rti_ifp;
struct rt_msghdr *rti_rtm;
u_char rti_mpls;
};
Index: net/rtsock.c
===
RCS file: /home/ncvs/src/sys/net/rtsock.c,v
retrieving revision 1.142
diff -u -p -r1.142 rtsock.c
--- net/rtsock.c 18 Mar 2014 10:47:34 - 1.142
+++ net/rtsock.c 24 Apr 2014 12:45:04 -
@@ -768,7 +768,7 @@ report:
ifafree(rt-rt_ifa);
rt-rt_ifa = ifa;
ifa-ifa_refcnt++;
- rt-rt_ifp = info.rti_ifp;
+ rt-rt_ifp = ifa-ifa_ifp;
#ifndef SMALL_KERNEL
/* recheck link state after ifp
Re: Remove rti_ifp from struct rt_addrinfo
On Thu, Apr 24, 2014 at 01:43:16PM +0200, Henning Brauer wrote: * Martin Pieuchot mpieuc...@nolizard.org [2014-04-24 13:24]: This ifp pointer is only needed by rt_getifa() to find an address, so make it a local variable. The rtrequest1(9) change might introduce a negligible slowdown since I remove the already known ifp pointer. But this only happens in the case described in the comment just before and I would bet because of carp_setroute(), still nobody to fix this? It's not better than OpenSSL... In the rtsock chunk, the two pointers are equivalent. Ok? yup. the carp route fiddling is pretty damn mad, and with the route priorities and the ability to mark routes as down there should be a much cleaner way to do this these days. heck, the entire carp route fiddling needs to be reassesed. things changed, i can\t even fully remeber why it is there (i think it was about backup nodes still being able to reach a network only present on the carp if or the like), and i seem to remember it doesn't quite work as expected anyway, but don't take my word for it, memory REALLY fuzzy on that front. Years ago mpf@ told me, that the purpose of this function is to ssh from a carp master to a carp slave via the carp address. Or was it the other way around? The carp_setroute() function starts with the encouraging comment /* XXX this mess needs fixing */. When switching carp states fast, the routing table gets messed up. In our product we removed all calls to carp_setroute(sc, RTM_DELETE). There is one carp_setroute(sc, RTM_ADD) left. I don't know wether it is needed. I would recommend to try to delete the whole function and fix fallout if any. bluhm
Re: Remove rti_ifp from struct rt_addrinfo
* Alexander Bluhm alexander.bl...@gmx.net [2014-04-24 18:18]: The carp_setroute() function starts with the encouraging comment /* XXX this mess needs fixing */. I didn't change my mind at least :) 1.136(henning 04-May-07): /* XXX this mess needs fixing */ When switching carp states fast, the routing table gets messed up. In our product we removed all calls to carp_setroute(sc, RTM_DELETE). There is one carp_setroute(sc, RTM_ADD) left. I don't know wether it is needed. I would recommend to try to delete the whole function and fix fallout if any. I tend towards that. ryan, marco? -- Henning Brauer, h...@bsws.de, henn...@openbsd.org BS Web Services GmbH, http://bsws.de, Full-Service ISP Secure Hosting, Mail and DNS Services. Dedicated Servers, Root to Fully Managed Henning Brauer Consulting, http://henningbrauer.com/
Re: Remove rti_ifp from struct rt_addrinfo
On 24/04/14(Thu) 18:17, Alexander Bluhm wrote: On Thu, Apr 24, 2014 at 01:43:16PM +0200, Henning Brauer wrote: * Martin Pieuchot mpieuc...@nolizard.org [2014-04-24 13:24]: This ifp pointer is only needed by rt_getifa() to find an address, so make it a local variable. The rtrequest1(9) change might introduce a negligible slowdown since I remove the already known ifp pointer. But this only happens in the case described in the comment just before and I would bet because of carp_setroute(), still nobody to fix this? It's not better than OpenSSL... In the rtsock chunk, the two pointers are equivalent. Ok? yup. the carp route fiddling is pretty damn mad, and with the route priorities and the ability to mark routes as down there should be a much cleaner way to do this these days. heck, the entire carp route fiddling needs to be reassesed. things changed, i can\t even fully remeber why it is there (i think it was about backup nodes still being able to reach a network only present on the carp if or the like), and i seem to remember it doesn't quite work as expected anyway, but don't take my word for it, memory REALLY fuzzy on that front. Years ago mpf@ told me, that the purpose of this function is to ssh from a carp master to a carp slave via the carp address. Or was it the other way around? The carp_setroute() function starts with the encouraging comment /* XXX this mess needs fixing */. When switching carp states fast, the routing table gets messed up. In our product we removed all calls to carp_setroute(sc, RTM_DELETE). There is one carp_setroute(sc, RTM_ADD) left. I don't know wether it is needed. I would recommend to try to delete the whole function and fix fallout if any. I totally support this idea. I even briefly tried that some months ago without seeing any breakage for *my* use case of carp.
Re: Remove rti_ifp from struct rt_addrinfo
On Thu, Apr 24, 2014 at 06:25:59PM +0200, Henning Brauer wrote: * Alexander Bluhm alexander.bl...@gmx.net [2014-04-24 18:18]: The carp_setroute() function starts with the encouraging comment /* XXX this mess needs fixing */. I didn't change my mind at least :) 1.136(henning 04-May-07): /* XXX this mess needs fixing */ When switching carp states fast, the routing table gets messed up. In our product we removed all calls to carp_setroute(sc, RTM_DELETE). There is one carp_setroute(sc, RTM_ADD) left. I don't know wether it is needed. I would recommend to try to delete the whole function and fix fallout if any. I tend towards that. ryan, marco? I think I have not lookd at that code in nearly 10 years, I will need more coffee. Maybe someone can summon the ghost of pasco@, as he did most of the cleanup in the 3.5-3.6 series of major changes. Part of the reason it's there is to make carp work properly for services listening on the carp interface, in particular so that hosts in the BACKUP state will reach the MASTER rather than trying and failing to connect to their own carp interface. Maybe not needed in all setups, but likely to break things if we simply remove it.
Re: Remove rti_ifp from struct rt_addrinfo
On Fri, Apr 25, 2014 at 09:09:03AM +0900, Ryan McBride wrote: Part of the reason it's there is to make carp work properly for services listening on the carp interface, in particular so that hosts in the BACKUP state will reach the MASTER rather than trying and failing to connect to their own carp interface. Maybe not needed in all setups, but likely to break things if we simply remove it. Why do you want to connect from the BACKUP machine to the MASTER using CARP addresses? Just add another fixed address and you can do that. The current implementation may change the routing table in subtile ways until nothing works. In IPv6 the routes are fixed and there are less problems. bluhm
