On Tue, Aug 15, 2017 at 02:37:22PM -0400, Jeremie Courreges-Anglas wrote:
> On Tue, Aug 15 2017, Rob Pierce <r...@2keys.ca> wrote:
> > On Mon, Aug 14, 2017 at 11:26:46PM -0400, Jeremie Courreges-Anglas wrote:
> >> On Mon, Aug 14 2017, Rob Pierce <r...@2keys.ca> 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 -0000 1.59
> >> +++ ifstated.c 15 Aug 2017 03:04:47 -0000
> >> @@ -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 *)&msg;
> >> 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(&ifm, 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(&expressions);
> >>
> >> TAILQ_FOREACH(ifstate, &state->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, &conf->initstate) && do_eval)
> >> + if (scan_ifstate_single(ifname, s, &conf->initstate) && do_eval)
> >> eval_state(&conf->initstate);
> >> TAILQ_FOREACH(state, &conf->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
> >> ===================================================================
> >> RCS file: /d/cvs/src/usr.sbin/ifstated/ifstated.h,v
> >> retrieving revision 1.18
> >> diff -u -p -r1.18 ifstated.h
> >> --- ifstated.h 14 Aug 2017 03:15:28 -0000 1.18
> >> +++ ifstated.h 15 Aug 2017 03:04:47 -0000
> >> @@ -41,7 +41,7 @@ struct ifsd_ifstate {
> >> #define IFSD_LINKUP 2
> >> int prevstate;
> >> u_int32_t refcount;
> >> - u_short ifindex;
> >> + char ifname[IFNAMSIZ];
> >> };
> >>
> >> struct ifsd_external {
> >> Index: parse.y
> >> ===================================================================
> >> RCS file: /d/cvs/src/usr.sbin/ifstated/parse.y,v
> >> retrieving revision 1.45
> >> diff -u -p -r1.45 parse.y
> >> --- parse.y 23 Jul 2017 13:53:54 -0000 1.45
> >> +++ parse.y 15 Aug 2017 03:23:00 -0000
> >> @@ -85,7 +85,7 @@ struct ifsd_state *curstate;
> >> void link_states(struct ifsd_action *);
> >> void set_expression_depth(struct ifsd_expression *,
> >> int);
> >> void init_state(struct ifsd_state *);
> >> -struct ifsd_ifstate *new_ifstate(u_short, int);
> >> +struct ifsd_ifstate *new_ifstate(char *, int);
> >> struct ifsd_external *new_external(char *, u_int32_t);
> >>
> >> typedef struct {
> >> @@ -93,7 +93,6 @@ typedef struct {
> >> int64_t number;
> >> char *string;
> >> struct in_addr addr;
> >> - u_short interface;
> >>
> >> struct ifsd_expression *expression;
> >> struct ifsd_ifstate *ifstate;
> >> @@ -114,7 +113,7 @@ typedef struct {
> >> %token <v.string> STRING
> >> %token <v.number> NUMBER
> >> %type <v.string> string
> >> -%type <v.interface> interface
> >> +%type <v.string> interface
> >> %type <v.ifstate> if_test
> >> %type <v.external> ext_test
> >> %type <v.expression> expr term
> >> @@ -170,12 +169,12 @@ conf_main : INITSTATE STRING {
> >> ;
> >>
> >> interface : STRING {
> >> - if (($$ = if_nametoindex($1)) == 0) {
> >> + if (if_nametoindex($1) == 0) {
> >> yyerror("unknown interface %s", $1);
> >> free($1);
> >> YYERROR;
> >> }
> >> - free($1);
> >> + $$ = $1;
> >> }
> >> ;
> >>
> >> @@ -933,7 +932,7 @@ init_state(struct ifsd_state *state)
> >> }
> >>
> >> struct ifsd_ifstate *
> >> -new_ifstate(u_short ifindex, int s)
> >> +new_ifstate(char *ifname, int s)
> >> {
> >> struct ifsd_ifstate *ifstate = NULL;
> >> struct ifsd_state *state;
> >> @@ -944,12 +943,16 @@ new_ifstate(u_short ifindex, int s)
> >> state = &conf->initstate;
> >>
> >> TAILQ_FOREACH(ifstate, &state->interface_states, entries)
> >> - if (ifstate->ifindex == ifindex && ifstate->ifstate == s)
> >> + if (strcmp(ifstate->ifname, ifname) == 0 &&
> >> + ifstate->ifstate == s)
> >> break;
> >> if (ifstate == NULL) {
> >> if ((ifstate = calloc(1, sizeof(*ifstate))) == NULL)
> >> err(1, NULL);
> >> - ifstate->ifindex = ifindex;
>
> >> + if (strlcpy(ifstate->ifname, ifname,
> >> + sizeof(ifstate->ifname)) == 0)
> >> + errx(1, "ifname strlcpy truncation");
>
> The test doesn't look correct, rather:
>
> if (strlcpy(ifstate->ifname, ifname,
> sizeof(ifstate->ifname)) >= sizeof(ifstate->ifname))
> errx(1, "ifname strlcpy truncation");
>
> >> + free(ifname);
> >> ifstate->ifstate = s;
> >> TAILQ_INIT(&ifstate->expressions);
> >> TAILQ_INSERT_TAIL(&state->interface_states, ifstate, entries);
> >>
> >> --
> >> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
> >
>
> --
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
