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 -0000 1.228
+++ netinet/ip_carp.c 28 Apr 2014 12:59:17 -0000
@@ -198,7 +198,6 @@ void carp_hmac_generate(struct carp_vhos
unsigned char *, u_int8_t);
int carp_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 &&
- rt->rt_flags & (RTF_CLONING|RTF_CLONED));
-
- /* Check for a network route on our interface */
- bcopy(ifa->ifa_addr, &sa, sizeof(sa));
- satosin(&sa)->sin_addr.s_addr = satosin(ifa->ifa_netmask
- )->sin_addr.s_addr & satosin(&sa)->sin_addr.s_addr;
- rt = rt_lookup(&sa,
- ifa->ifa_netmask, sc->sc_if.if_rdomain);
- nr_ourif = (rt && rt->rt_ifp == &sc->sc_if);
-
- /* Restore the route label */
- memset(&sa_rl, 0, sizeof(sa_rl));
- if (rt && rt->rt_labelid) {
- sa_rl.sr_len = sizeof(sa_rl);
- sa_rl.sr_family = AF_UNSPEC;
- label = rtlabel_id2name(rt->rt_labelid);
- if (label != NULL)
- strlcpy(sa_rl.sr_label, label,
- sizeof(sa_rl.sr_label));
- }
-
- switch (cmd) {
- case RTM_ADD:
- if (hr_otherif) {
- ifa->ifa_rtrequest = NULL;
- memset(&info, 0, sizeof(info));
- info.rti_info[RTAX_DST] = ifa->ifa_addr;
- info.rti_info[RTAX_GATEWAY] =
ifa->ifa_addr;
- info.rti_flags = RTF_UP | RTF_HOST;
- error = rtrequest1(RTM_ADD, &info,
- RTP_CONNECTED, NULL,
- sc->sc_if.if_rdomain);
- rt_missmsg(RTM_ADD, &info,
- info.rti_flags, &sc->sc_if,
- error, sc->sc_if.if_rdomain);
- }
- if (!hr_otherif || nr_ourif || !rt) {
- if (nr_ourif && !(rt->rt_flags &
- RTF_CLONING)) {
- memset(&info, 0, sizeof(info));
- info.rti_info[RTAX_DST] = &sa;
- info.rti_info[RTAX_NETMASK] =
ifa->ifa_netmask;
- 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);
- }
-
- ifa->ifa_rtrequest = arp_rtrequest;
-
- memset(&info, 0, sizeof(info));
- info.rti_info[RTAX_DST] = &sa;
- info.rti_info[RTAX_GATEWAY] =
ifa->ifa_addr;
- info.rti_info[RTAX_NETMASK] =
ifa->ifa_netmask;
- info.rti_info[RTAX_LABEL] =
- (struct sockaddr *)&sa_rl;
- error = rtrequest1(RTM_ADD, &info,
- RTP_CONNECTED, NULL,
- sc->sc_if.if_rdomain);
- if (error == 0)
- ifa->ifa_flags |= IFA_ROUTE;
- rt_missmsg(RTM_ADD, &info,
info.rti_flags,
- &sc->sc_if, error,
sc->sc_if.if_rdomain);
- }
- break;
- case RTM_DELETE:
- break;
- default:
- break;
- }
- break;
- }
- default:
- break;
- }
- }
- splx(s);
-}
-
/*
* process input packet.
* we have rearranged checks order compared to the rfc,
@@ -749,8 +630,6 @@ carp_proto_input_c(struct mbuf *m, struc
timeout_del(&vhe->ad_tmo);
carp_set_state(vhe, BACKUP);
carp_setrun(vhe, 0);
- if (vhe->vhe_leader)
- carp_setroute(sc, RTM_DELETE);
}
break;
case BACKUP:
@@ -1655,8 +1534,6 @@ carp_master_down(void *v)
#endif /* INET6 */
}
carp_setrun(vhe, 0);
- if (vhe->vhe_leader)
- carp_setroute(sc, RTM_ADD);
carpstats.carps_preempt++;
break;
}
@@ -1698,16 +1575,12 @@ carp_setrun(struct carp_vhost_entry *vhe
sc->sc_if.if_flags |= IFF_RUNNING;
} else {
sc->sc_if.if_flags &= ~IFF_RUNNING;
- if (vhe->vhe_leader)
- carp_setroute(sc, RTM_DELETE);
return;
}
switch (vhe->state) {
case INIT:
carp_set_state(vhe, BACKUP);
- if (vhe->vhe_leader)
- carp_setroute(sc, RTM_DELETE);
carp_setrun(vhe, 0);
break;
case BACKUP:
@@ -2277,7 +2150,6 @@ carp_ioctl(struct ifnet *ifp, u_long cmd
timeout_del(&vhe->ad_tmo);
carp_set_state_all(sc, BACKUP);
carp_setrun_all(sc, 0);
- carp_setroute(sc, RTM_DELETE);
break;
case MASTER:
LIST_FOREACH(vhe, &sc->carp_vhosts,
