On Tue, Aug 08, 2017 at 12:12:43AM +0200, Jeremie Courreges-Anglas wrote:
> On Sun, Aug 06 2017, Rob Pierce <[email protected]> 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 <[email protected]>
> > @@ -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
