On Tue, Aug 08, 2017 at 12:12:43AM +0200, Jeremie Courreges-Anglas wrote: > On Sun, Aug 06 2017, Rob Pierce <r...@2keys.ca> wrote: > > The following diff adds support for detecting the state change of a departed > > interface. ifstated is not a very verbose daemon, so this diff quietly does > > the right thing (i.e. there is no exttra warning about a departing > > interface). > > But maybe there should be at least a big scary message. This is not > exactly a normal situation.
Yes, I agree. This is a special case. > > The re-arrival of a departed interface involves re-indexing the interface > > and > > possibly other complexities that require more consideration, but for now at > > least this obvious condition is handled in what I believe is a more > > appropriate manner. > > I wonder what's the most useful behavior here: complain loudly and fail > hard (exit) or just consider the link down and monitor the rest of the > remaining interfaces, as in your diff. I do like the idea of complaining loudly and failing hard. I had originally considered a fatal (after forcing a demote of course). > Destroying then reconfiguring a carp(4) interface won't give you a very > user-friendly behavior. Same thing with disconnecting/reconnecting > interfaces like urtwn(4) or urndis(4). ifstated(8) won't do anything > about your new interface, even if you checked that said interface was > in good shape. > > Also if we keep running, the link is now considered down and we're > likely to execute commands that refer to an interface that has left; > that might be good or bad, I don't know what people put in their > ifstated.conf. > > > Updated regression tests pass, and the corresponding regression diff is also > > attached. > > > > Ok? > > I'm not sure yet which path we should follow. Let's discuss this in > Toronto, shal we? In the meantime, please see the nits below. Sounds like a plan. Looking forward to it! Rob > > Index: regress/usr.sbin/ifstated/ifstated > > =================================================================== > > RCS file: /cvs/src/regress/usr.sbin/ifstated/ifstated,v > > retrieving revision 1.3 > > diff -u -p -r1.3 ifstated > > --- regress/usr.sbin/ifstated/ifstated 31 Jul 2017 18:41:21 -0000 > > 1.3 > > +++ regress/usr.sbin/ifstated/ifstated 6 Aug 2017 23:29:11 -0000 > > @@ -124,6 +124,7 @@ changing state to primary > > changing state to demoted > > changing state to primary > > changing state to primary > > +changing state to demoted > > EOF > > > > (cd working && nohup ifstated -dvf ./ifstated.conf > ifstated.log 2>&1) & > > @@ -148,6 +149,8 @@ ifconfig carp${VHIDB} inet ${PREFIX}.${V > > ${PREFIX}.255 vhid ${VHIDB} carpdev ${NIC} > > sleep ${SLEEP} > > kill -HUP $(pgrep ifstated) >/dev/null 2>&1 > > +sleep ${SLEEP} > > +ifconfig carp${VHIDA} destroy > > sleep ${SLEEP} > > > > grep ^changing working/ifstated.log > working/output.new > > Index: usr.sbin/ifstated/ifstated.c > > =================================================================== > > RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v > > retrieving revision 1.57 > > diff -u -p -r1.57 ifstated.c > > --- usr.sbin/ifstated/ifstated.c 6 Aug 2017 19:27:54 -0000 1.57 > > +++ usr.sbin/ifstated/ifstated.c 6 Aug 2017 23:29:12 -0000 > > @@ -1,4 +1,4 @@ > > -/* $OpenBSD: ifstated.c,v 1.57 2017/08/06 19:27:54 rob Exp $ */ > > +/* $OpenBSD: ifstated.c,v 1.56 2017/07/24 12:33:59 jca Exp $ */ > > > > /* > > * Copyright (c) 2004 Marco Pfatschbacher <m...@openbsd.org> > > @@ -61,6 +61,7 @@ void rt_msg_handler(int, short, void *) > > void external_handler(int, short, void *); > > void external_exec(struct ifsd_external *, int); > > void check_external_status(struct ifsd_state *); > > +void check_for_ifdeparture(void); > > check_ifdeparture() would be shorter and just as descriptive. > > > void external_evtimer_setup(struct ifsd_state *, int); > > void scan_ifstate(int, int, int); > > int scan_ifstate_single(int, int, struct ifsd_state *); > > @@ -150,7 +151,7 @@ main(int argc, char *argv[]) > > if ((rt_fd = socket(PF_ROUTE, SOCK_RAW, 0)) < 0) > > err(1, "no routing socket"); > > > > - rtfilter = ROUTE_FILTER(RTM_IFINFO); > > + rtfilter = ROUTE_FILTER(RTM_IFINFO) | ROUTE_FILTER(RTM_IFANNOUNCE); > > if (setsockopt(rt_fd, PF_ROUTE, ROUTE_MSGFILTER, > > &rtfilter, sizeof(rtfilter)) == -1) /* not fatal */ > > log_warn("%s: setsockopt msgfilter", __func__); > > @@ -234,6 +235,7 @@ rt_msg_handler(int fd, short event, void > > char msg[2048]; > > struct rt_msghdr *rtm = (struct rt_msghdr *)&msg; > > struct if_msghdr ifm; > > + struct if_announcemsghdr ifan; > > ssize_t len; > > > > if ((len = read(fd, msg, sizeof(msg))) == -1) { > > @@ -253,8 +255,19 @@ rt_msg_handler(int fd, short event, void > > memcpy(&ifm, rtm, sizeof(ifm)); > > scan_ifstate(ifm.ifm_index, ifm.ifm_data.ifi_link_state, 1); > > break; > > + case RTM_IFANNOUNCE: > > + memcpy(&ifan, rtm, sizeof(ifan)); > > + switch (ifan.ifan_what) { > > + case IFAN_DEPARTURE: > > + scan_ifstate(ifan.ifan_index, LINK_STATE_DOWN, 1); > > + break; > > + default: > > + break; > > + } > > + break; > > An "if (ifan.ifan_what == IFAN_DEPARTURE)" test would be shorter. > > > case RTM_DESYNC: > > fetch_ifstate(1); > > + check_for_ifdeparture(); > > break; > > default: > > break; > > I don't think there's much point in having an empty default: entry. > > > @@ -626,6 +639,26 @@ fetch_ifstate(int do_eval) > > } > > > > freeifaddrs(ifap); > > +} > > + > > +void > > +check_for_ifdeparture(void) > > +{ > > + struct ifsd_state *state; > > + struct ifsd_ifstate *ifstate; > > + char ifnamebuf[IF_NAMESIZE]; > > + char *if_name; > > + > > + TAILQ_FOREACH(state, &conf->states, entries) { > > + TAILQ_FOREACH(ifstate, &state->interface_states, entries) { > > + if_name = if_indextoname(ifstate->ifindex, ifnamebuf); > > + if (if_name == NULL) { > > + scan_ifstate(ifstate->ifindex, > > + LINK_STATE_DOWN, 1); > > + } > > + } > > + } > > + return; > > Like in rt_msg_handler() this return statement isn't needed. > > > } > > > > void > > > > -- > jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE