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