Re: ifstated: add handing of departed interfaces

[Rob Pierce]

On Tue, Aug 08, 2017 at 12:12:43AM +0200, Jeremie Courreges-Anglas wrote:
> On Sun, Aug 06 2017, Rob Pierce  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 -  
> > 1.3
> > +++ regress/usr.sbin/ifstated/ifstated  6 Aug 2017 23:29:11 -
> > @@ -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.c6 Aug 2017 19:27:54 -   1.57
> > +++ usr.sbin/ifstated/ifstated.c6 Aug 2017 23:29:12 -
> > @@ -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 
> > @@ -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);
> >  intscan_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,
> > , 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 *)
> > 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
> > 

Re: ifstated: add handing of departed interfaces

[Jeremie Courreges-Anglas]

On Sun, Aug 06 2017, Rob Pierce  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.

> 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.

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.

> 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/ifstated31 Jul 2017 18:41:21 -  
> 1.3
> +++ regress/usr.sbin/ifstated/ifstated6 Aug 2017 23:29:11 -
> @@ -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 -   1.57
> +++ usr.sbin/ifstated/ifstated.c  6 Aug 2017 23:29:12 -
> @@ -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 
> @@ -61,6 +61,7 @@ voidrt_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,
>   , 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 *)
>   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(, rtm, sizeof(ifm));
>   scan_ifstate(ifm.ifm_index, ifm.ifm_data.ifi_link_state, 1);
>   break;
> + case RTM_IFANNOUNCE:
> + memcpy(, 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 ==