On Mon, Aug 14, 2017 at 11:26:46PM -0400, Jeremie Courreges-Anglas wrote:
> On Mon, Aug 14 2017, Rob Pierce <[email protected]> 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.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");
> + 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
