Re: ifstated: stop tracking interface indexes

[Rob Pierce]

On Tue, Aug 15, 2017 at 02:37:22PM -0400, Jeremie Courreges-Anglas wrote:
> On Tue, Aug 15 2017, Rob Pierce  wrote:
> > On Mon, Aug 14, 2017 at 11:26:46PM -0400, Jeremie Courreges-Anglas wrote:
> >> On Mon, Aug 14 2017, Rob Pierce  wrote:
> >> > ifstated currently tracks and maintains the index of each monitored 
> >> > interface
> >> > and does not maintain interface names. This means we need to re-index on
> >> > interface departure and arrival.
> >> >
> >> > The following diff moves away from indexes to names. Indexes are still 
> >> > required,
> >> > but easily obtained dynamically as needed. This helps simplify the next 
> >> > diff
> >> > that will provide support for interface departure and arrival.
> >> >
> >> > Suggested by deraadt.
> >> >
> >> > No intended functional change. Regress tests pass.
> >> >
> >> > Ok?
> >> 
> >> The idea looks sound to me, however I would keep the "interface" symbol
> >> in parse.y (your diff doesn't remove all "interface" references btw).
> >> 
> >> The current code checks the existence of the interface at startup.  If
> >> the interface doesn't exists, you get a syntax error.  This could happen
> >> because of a missing interface (an interesting case), or because of
> >> a typo.  Whether or not we're erroring out, it is nice to print
> >> a diagnostic message.
> >> 
> >> I'm not sure this change was intended, so here's a tentative diff that
> >> that keeps the existing behavior.  Regress tests pass.
> >
> > Yes, I see that now. That change was not intended. Thanks!
> >
> > Your diff looks good.
> >
> > Ok?
> 
> Please use four spaces, not three, for second level indents; see
> style(9).
> 
> There's a check that should be fixed, please see below.

Ok, will do. Not sure why I started using three spaces for 2nd level indents...

> With those items addressed, ok jca@
> 
> It feels a bit weird to reject unknown interface names at startup but to
> cope with departed/joining interfaces at runtime.  But I guess we'll
> have to live with this.

I don't disagree. I am most concerned about handling the departure condition
more appropriately, which is (in my opinion) currently broken.

As discussed, it is an option to do the right thing on departure (i.e. demote
with a state change to down) and then fail hard, which would mirror the
(current) startup behaviour more closely (i.e. fail on startup due to the
interfaces not being present).

I suppose we could also allow startup with unknown interfaces (present in the
configuration file) to occur (which would be a change in behaviour), and simply
mark those interfaces as down and immediately demote but keep running, like we
do for an interface that is present at startup but in a down state.

This diff does not yet introduce interface departure and arrival, and with
your updates will not change any behaviour, and I think this change makes
sense regardless of which approach is taken in the next step.

Thank you for your thorough review and comments. I will update my diff
accordingly and wait a few days for any further comments before committing.

Regards,

Rob

> >> Index: ifstated.c
> >> ===
> >> RCS file: /d/cvs/src/usr.sbin/ifstated/ifstated.c,v
> >> retrieving revision 1.59
> >> diff -u -p -r1.59 ifstated.c
> >> --- ifstated.c 14 Aug 2017 03:15:28 -  1.59
> >> +++ ifstated.c 15 Aug 2017 03:04:47 -
> >> @@ -61,8 +61,8 @@ void external_handler(int, short, void 
> >>  void  external_exec(struct ifsd_external *, int);
> >>  void  check_external_status(struct ifsd_state *);
> >>  void  external_evtimer_setup(struct ifsd_state *, int);
> >> -void  scan_ifstate(int, int, int);
> >> -int   scan_ifstate_single(int, int, struct ifsd_state *);
> >> +void  scan_ifstate(const char *, int, int);
> >> +int   scan_ifstate_single(const char *, int, struct 
> >> ifsd_state *);
> >>  void  fetch_ifstate(int);
> >>  __dead void   usage(void);
> >>  void  adjust_expressions(struct ifsd_expression_list *, int);
> >> @@ -233,6 +233,8 @@ rt_msg_handler(int fd, short event, void
> >>char msg[2048];
> >>struct rt_msghdr *rtm = (struct rt_msghdr *)
> >>struct if_msghdr ifm;
> >> +  char ifnamebuf[IFNAMSIZ];
> >> +  char *ifname;
> >>ssize_t len;
> >>  
> >>if ((len = read(fd, msg, sizeof(msg))) == -1) {
> >> @@ -250,7 +252,10 @@ rt_msg_handler(int fd, short event, void
> >>switch (rtm->rtm_type) {
> >>case RTM_IFINFO:
> >>memcpy(, rtm, sizeof(ifm));
> >> -  scan_ifstate(ifm.ifm_index, ifm.ifm_data.ifi_link_state, 1);
> >> +  ifname = if_indextoname(ifm.ifm_index, ifnamebuf);
> >> +  /* ifname is NULL on interface departure */
> >> +  if (ifname != NULL)
> >> +  scan_ifstate(ifname, ifm.ifm_data.ifi_link_state, 1);
> >>

Re: ifstated: stop tracking interface indexes

[Jeremie Courreges-Anglas]

On Tue, Aug 15 2017, Rob Pierce  wrote:
> On Mon, Aug 14, 2017 at 11:26:46PM -0400, Jeremie Courreges-Anglas wrote:
>> On Mon, Aug 14 2017, Rob Pierce  wrote:
>> > ifstated currently tracks and maintains the index of each monitored 
>> > interface
>> > and does not maintain interface names. This means we need to re-index on
>> > interface departure and arrival.
>> >
>> > The following diff moves away from indexes to names. Indexes are still 
>> > required,
>> > but easily obtained dynamically as needed. This helps simplify the next 
>> > diff
>> > that will provide support for interface departure and arrival.
>> >
>> > Suggested by deraadt.
>> >
>> > No intended functional change. Regress tests pass.
>> >
>> > Ok?
>> 
>> The idea looks sound to me, however I would keep the "interface" symbol
>> in parse.y (your diff doesn't remove all "interface" references btw).
>> 
>> The current code checks the existence of the interface at startup.  If
>> the interface doesn't exists, you get a syntax error.  This could happen
>> because of a missing interface (an interesting case), or because of
>> a typo.  Whether or not we're erroring out, it is nice to print
>> a diagnostic message.
>> 
>> I'm not sure this change was intended, so here's a tentative diff that
>> that keeps the existing behavior.  Regress tests pass.
>
> Yes, I see that now. That change was not intended. Thanks!
>
> Your diff looks good.
>
> Ok?

Please use four spaces, not three, for second level indents; see
style(9).

There's a check that should be fixed, please see below.

With those items addressed, ok jca@

It feels a bit weird to reject unknown interface names at startup but to
cope with departed/joining interfaces at runtime.  But I guess we'll
have to live with this.

>> Index: ifstated.c
>> ===
>> RCS file: /d/cvs/src/usr.sbin/ifstated/ifstated.c,v
>> retrieving revision 1.59
>> diff -u -p -r1.59 ifstated.c
>> --- ifstated.c   14 Aug 2017 03:15:28 -  1.59
>> +++ ifstated.c   15 Aug 2017 03:04:47 -
>> @@ -61,8 +61,8 @@ void   external_handler(int, short, void 
>>  voidexternal_exec(struct ifsd_external *, int);
>>  voidcheck_external_status(struct ifsd_state *);
>>  voidexternal_evtimer_setup(struct ifsd_state *, int);
>> -voidscan_ifstate(int, int, int);
>> -int scan_ifstate_single(int, int, struct ifsd_state *);
>> +voidscan_ifstate(const char *, int, int);
>> +int scan_ifstate_single(const char *, int, struct ifsd_state *);
>>  voidfetch_ifstate(int);
>>  __dead void usage(void);
>>  voidadjust_expressions(struct ifsd_expression_list *, int);
>> @@ -233,6 +233,8 @@ rt_msg_handler(int fd, short event, void
>>  char msg[2048];
>>  struct rt_msghdr *rtm = (struct rt_msghdr *)
>>  struct if_msghdr ifm;
>> +char ifnamebuf[IFNAMSIZ];
>> +char *ifname;
>>  ssize_t len;
>>  
>>  if ((len = read(fd, msg, sizeof(msg))) == -1) {
>> @@ -250,7 +252,10 @@ rt_msg_handler(int fd, short event, void
>>  switch (rtm->rtm_type) {
>>  case RTM_IFINFO:
>>  memcpy(, rtm, sizeof(ifm));
>> -scan_ifstate(ifm.ifm_index, ifm.ifm_data.ifi_link_state, 1);
>> +ifname = if_indextoname(ifm.ifm_index, ifnamebuf);
>> +/* ifname is NULL on interface departure */
>> +if (ifname != NULL)
>> +scan_ifstate(ifname, ifm.ifm_data.ifi_link_state, 1);
>>  break;
>>  case RTM_DESYNC:
>>  fetch_ifstate(1);
>> @@ -431,7 +436,7 @@ external_evtimer_setup(struct ifsd_state
>>  #define LINK_STATE_IS_DOWN(_s)  (!LINK_STATE_IS_UP((_s)))
>>  
>>  int
>> -scan_ifstate_single(int ifindex, int s, struct ifsd_state *state)
>> +scan_ifstate_single(const char *ifname, int s, struct ifsd_state *state)
>>  {
>>  struct ifsd_ifstate *ifstate;
>>  struct ifsd_expression_list expressions;
>> @@ -440,7 +445,7 @@ scan_ifstate_single(int ifindex, int s, 
>>  TAILQ_INIT();
>>  
>>  TAILQ_FOREACH(ifstate, >interface_states, entries) {
>> -if (ifstate->ifindex == ifindex) {
>> +if (strcmp(ifstate->ifname, ifname) == 0) {
>>  if (ifstate->prevstate != s &&
>>  (ifstate->prevstate != -1 || !opt_inhibit)) {
>>  struct ifsd_expression *expression;
>> @@ -472,15 +477,15 @@ scan_ifstate_single(int ifindex, int s, 
>>  }
>>  
>>  void
>> -scan_ifstate(int ifindex, int s, int do_eval)
>> +scan_ifstate(const char *ifname, int s, int do_eval)
>>  {
>>  struct ifsd_state *state;
>>  int cur_eval = 0;
>>  
>> -if (scan_ifstate_single(ifindex, s, >initstate) && do_eval)
>> +if (scan_ifstate_single(ifname, s, >initstate) && do_eval)
>>  eval_state(>initstate);
>>  

Re: ifstated: stop tracking interface indexes

[Rob Pierce]

On Mon, Aug 14, 2017 at 11:26:46PM -0400, Jeremie Courreges-Anglas wrote:
> On Mon, Aug 14 2017, Rob Pierce  wrote:
> > ifstated currently tracks and maintains the index of each monitored 
> > interface
> > and does not maintain interface names. This means we need to re-index on
> > interface departure and arrival.
> >
> > The following diff moves away from indexes to names. Indexes are still 
> > required,
> > but easily obtained dynamically as needed. This helps simplify the next diff
> > that will provide support for interface departure and arrival.
> >
> > Suggested by deraadt.
> >
> > No intended functional change. Regress tests pass.
> >
> > Ok?
> 
> The idea looks sound to me, however I would keep the "interface" symbol
> in parse.y (your diff doesn't remove all "interface" references btw).
> 
> The current code checks the existence of the interface at startup.  If
> the interface doesn't exists, you get a syntax error.  This could happen
> because of a missing interface (an interesting case), or because of
> a typo.  Whether or not we're erroring out, it is nice to print
> a diagnostic message.
> 
> I'm not sure this change was intended, so here's a tentative diff that
> that keeps the existing behavior.  Regress tests pass.

Yes, I see that now. That change was not intended. Thanks!

Your diff looks good.

Ok?

> Index: ifstated.c
> ===
> RCS file: /d/cvs/src/usr.sbin/ifstated/ifstated.c,v
> retrieving revision 1.59
> diff -u -p -r1.59 ifstated.c
> --- ifstated.c14 Aug 2017 03:15:28 -  1.59
> +++ ifstated.c15 Aug 2017 03:04:47 -
> @@ -61,8 +61,8 @@ voidexternal_handler(int, short, void 
>  void external_exec(struct ifsd_external *, int);
>  void check_external_status(struct ifsd_state *);
>  void external_evtimer_setup(struct ifsd_state *, int);
> -void scan_ifstate(int, int, int);
> -int  scan_ifstate_single(int, int, struct ifsd_state *);
> +void scan_ifstate(const char *, int, int);
> +int  scan_ifstate_single(const char *, int, struct ifsd_state *);
>  void fetch_ifstate(int);
>  __dead void  usage(void);
>  void adjust_expressions(struct ifsd_expression_list *, int);
> @@ -233,6 +233,8 @@ rt_msg_handler(int fd, short event, void
>   char msg[2048];
>   struct rt_msghdr *rtm = (struct rt_msghdr *)
>   struct if_msghdr ifm;
> + char ifnamebuf[IFNAMSIZ];
> + char *ifname;
>   ssize_t len;
>  
>   if ((len = read(fd, msg, sizeof(msg))) == -1) {
> @@ -250,7 +252,10 @@ rt_msg_handler(int fd, short event, void
>   switch (rtm->rtm_type) {
>   case RTM_IFINFO:
>   memcpy(, rtm, sizeof(ifm));
> - scan_ifstate(ifm.ifm_index, ifm.ifm_data.ifi_link_state, 1);
> + ifname = if_indextoname(ifm.ifm_index, ifnamebuf);
> + /* ifname is NULL on interface departure */
> + if (ifname != NULL)
> + scan_ifstate(ifname, ifm.ifm_data.ifi_link_state, 1);
>   break;
>   case RTM_DESYNC:
>   fetch_ifstate(1);
> @@ -431,7 +436,7 @@ external_evtimer_setup(struct ifsd_state
>  #define  LINK_STATE_IS_DOWN(_s)  (!LINK_STATE_IS_UP((_s)))
>  
>  int
> -scan_ifstate_single(int ifindex, int s, struct ifsd_state *state)
> +scan_ifstate_single(const char *ifname, int s, struct ifsd_state *state)
>  {
>   struct ifsd_ifstate *ifstate;
>   struct ifsd_expression_list expressions;
> @@ -440,7 +445,7 @@ scan_ifstate_single(int ifindex, int s, 
>   TAILQ_INIT();
>  
>   TAILQ_FOREACH(ifstate, >interface_states, entries) {
> - if (ifstate->ifindex == ifindex) {
> + if (strcmp(ifstate->ifname, ifname) == 0) {
>   if (ifstate->prevstate != s &&
>   (ifstate->prevstate != -1 || !opt_inhibit)) {
>   struct ifsd_expression *expression;
> @@ -472,15 +477,15 @@ scan_ifstate_single(int ifindex, int s, 
>  }
>  
>  void
> -scan_ifstate(int ifindex, int s, int do_eval)
> +scan_ifstate(const char *ifname, int s, int do_eval)
>  {
>   struct ifsd_state *state;
>   int cur_eval = 0;
>  
> - if (scan_ifstate_single(ifindex, s, >initstate) && do_eval)
> + if (scan_ifstate_single(ifname, s, >initstate) && do_eval)
>   eval_state(>initstate);
>   TAILQ_FOREACH(state, >states, entries) {
> - if (scan_ifstate_single(ifindex, s, state) &&
> + if (scan_ifstate_single(ifname, s, state) &&
>   (do_eval && state == conf->curstate))
>   cur_eval = 1;
>   }
> @@ -619,8 +624,8 @@ fetch_ifstate(int do_eval)
>   for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
>   if (ifa->ifa_addr->sa_family == AF_LINK) {
>   struct if_data *ifdata = ifa->ifa_data;
> - 

Re: ifstated: stop tracking interface indexes

[Jeremie Courreges-Anglas]

On Mon, Aug 14 2017, Rob Pierce  wrote:
> ifstated currently tracks and maintains the index of each monitored interface
> and does not maintain interface names. This means we need to re-index on
> interface departure and arrival.
>
> The following diff moves away from indexes to names. Indexes are still 
> required,
> but easily obtained dynamically as needed. This helps simplify the next diff
> that will provide support for interface departure and arrival.
>
> Suggested by deraadt.
>
> No intended functional change. Regress tests pass.
>
> Ok?

The idea looks sound to me, however I would keep the "interface" symbol
in parse.y (your diff doesn't remove all "interface" references btw).

The current code checks the existence of the interface at startup.  If
the interface doesn't exists, you get a syntax error.  This could happen
because of a missing interface (an interesting case), or because of
a typo.  Whether or not we're erroring out, it is nice to print
a diagnostic message.

I'm not sure this change was intended, so here's a tentative diff that
that keeps the existing behavior.  Regress tests pass.


Index: ifstated.c
===
RCS file: /d/cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.59
diff -u -p -r1.59 ifstated.c
--- ifstated.c  14 Aug 2017 03:15:28 -  1.59
+++ ifstated.c  15 Aug 2017 03:04:47 -
@@ -61,8 +61,8 @@ void  external_handler(int, short, void 
 void   external_exec(struct ifsd_external *, int);
 void   check_external_status(struct ifsd_state *);
 void   external_evtimer_setup(struct ifsd_state *, int);
-void   scan_ifstate(int, int, int);
-intscan_ifstate_single(int, int, struct ifsd_state *);
+void   scan_ifstate(const char *, int, int);
+intscan_ifstate_single(const char *, int, struct ifsd_state *);
 void   fetch_ifstate(int);
 __dead voidusage(void);
 void   adjust_expressions(struct ifsd_expression_list *, int);
@@ -233,6 +233,8 @@ rt_msg_handler(int fd, short event, void
char msg[2048];
struct rt_msghdr *rtm = (struct rt_msghdr *)
struct if_msghdr ifm;
+   char ifnamebuf[IFNAMSIZ];
+   char *ifname;
ssize_t len;
 
if ((len = read(fd, msg, sizeof(msg))) == -1) {
@@ -250,7 +252,10 @@ rt_msg_handler(int fd, short event, void
switch (rtm->rtm_type) {
case RTM_IFINFO:
memcpy(, rtm, sizeof(ifm));
-   scan_ifstate(ifm.ifm_index, ifm.ifm_data.ifi_link_state, 1);
+   ifname = if_indextoname(ifm.ifm_index, ifnamebuf);
+   /* ifname is NULL on interface departure */
+   if (ifname != NULL)
+   scan_ifstate(ifname, ifm.ifm_data.ifi_link_state, 1);
break;
case RTM_DESYNC:
fetch_ifstate(1);
@@ -431,7 +436,7 @@ external_evtimer_setup(struct ifsd_state
 #defineLINK_STATE_IS_DOWN(_s)  (!LINK_STATE_IS_UP((_s)))
 
 int
-scan_ifstate_single(int ifindex, int s, struct ifsd_state *state)
+scan_ifstate_single(const char *ifname, int s, struct ifsd_state *state)
 {
struct ifsd_ifstate *ifstate;
struct ifsd_expression_list expressions;
@@ -440,7 +445,7 @@ scan_ifstate_single(int ifindex, int s, 
TAILQ_INIT();
 
TAILQ_FOREACH(ifstate, >interface_states, entries) {
-   if (ifstate->ifindex == ifindex) {
+   if (strcmp(ifstate->ifname, ifname) == 0) {
if (ifstate->prevstate != s &&
(ifstate->prevstate != -1 || !opt_inhibit)) {
struct ifsd_expression *expression;
@@ -472,15 +477,15 @@ scan_ifstate_single(int ifindex, int s, 
 }
 
 void
-scan_ifstate(int ifindex, int s, int do_eval)
+scan_ifstate(const char *ifname, int s, int do_eval)
 {
struct ifsd_state *state;
int cur_eval = 0;
 
-   if (scan_ifstate_single(ifindex, s, >initstate) && do_eval)
+   if (scan_ifstate_single(ifname, s, >initstate) && do_eval)
eval_state(>initstate);
TAILQ_FOREACH(state, >states, entries) {
-   if (scan_ifstate_single(ifindex, s, state) &&
+   if (scan_ifstate_single(ifname, s, state) &&
(do_eval && state == conf->curstate))
cur_eval = 1;
}
@@ -619,8 +624,8 @@ fetch_ifstate(int do_eval)
for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
if (ifa->ifa_addr->sa_family == AF_LINK) {
struct if_data *ifdata = ifa->ifa_data;
-   scan_ifstate(if_nametoindex(ifa->ifa_name),
-   ifdata->ifi_link_state, do_eval);
+   scan_ifstate(ifa->ifa_name, ifdata->ifi_link_state,
+  do_eval);
}
}
 
Index: ifstated.h