2015-03-11 22:30 GMT-03:00 Claudio Jeker <[email protected]>: > On Wed, Mar 11, 2015 at 04:41:08PM -0300, Renato Westphal wrote: >> In the name of simplicity, remove the interface FSM that was inherited >> from ospfd. In ldpd interfaces are just up or down, so keeping a FSM >> for that is an overkill. Now instead of calling if_fsm(), just call >> if_update() whenever a relevant event occurs (status change, address >> addition/removal). >> >> Additional notes: >> 1 - s/if_act_/if_/ >> >> 2 - Remove the IMSG_IFUP and IMSG_IFDOWN events. Now whenever an >> interface changes its state a IMSG_IFSTATUS event will be generated >> with the new status. >> --- >> interface.c | 118 >> +++++++++--------------------------------------------------- >> kroute.c | 13 ++++--- >> ldpd.h | 19 ---------- >> ldpe.c | 21 ++--------- >> ldpe.h | 4 ++- >> 5 files changed, 28 insertions(+), 147 deletions(-) >> >> diff --git a/interface.c b/interface.c >> index 7c80d1e..62c2766 100644 >> --- a/interface.c >> +++ b/interface.c >> @@ -41,98 +41,9 @@ >> >> extern struct ldpd_conf *leconf; >> >> -int if_act_start(struct iface *); >> -int if_act_reset(struct iface *); >> -int if_act_update(struct iface *); >> void if_hello_timer(int, short, void *); >> void if_start_hello_timer(struct iface *); >> void if_stop_hello_timer(struct iface *); >> -struct nbr *if_elect(struct nbr *, struct nbr *); >> - >> -struct { >> - int state; >> - enum iface_event event; >> - enum iface_action action; >> - int new_state; >> -} iface_fsm[] = { >> - /* current state event that happened action to take resulting >> state */ >> - {IF_STA_DOWN, IF_EVT_DOWN, IF_ACT_NOTHING, 0}, >> - {IF_STA_DOWN, IF_EVT_UP, IF_ACT_UPDATE, 0}, >> - {IF_STA_DOWN, IF_EVT_NEWADDR, IF_ACT_UPDATE, 0}, >> - {IF_STA_DOWN, IF_EVT_DELADDR, IF_ACT_NOTHING, 0}, >> - {IF_STA_ACTIVE, IF_EVT_DOWN, IF_ACT_RST, IF_STA_DOWN}, >> - {IF_STA_ACTIVE, IF_EVT_NEWADDR, IF_ACT_NOTHING, 0}, >> - {IF_STA_ACTIVE, IF_EVT_DELADDR, IF_ACT_UPDATE, 0}, >> - {-1, IF_EVT_NOTHING, IF_ACT_NOTHING, 0}, >> -}; >> - >> -const char * const if_event_names[] = { >> - "NOTHING", >> - "UP", >> - "DOWN", >> - "NEWADDR", >> - "DELADDR" >> -}; >> - >> -const char * const if_action_names[] = { >> - "NOTHING", >> - "UPDATE", >> - "RESET" >> -}; >> - >> -int >> -if_fsm(struct iface *iface, enum iface_event event) >> -{ >> - int old_state; >> - int new_state = 0; >> - int i, ret = 0; >> - >> - old_state = iface->state; >> - >> - for (i = 0; iface_fsm[i].state != -1; i++) >> - if ((iface_fsm[i].state & old_state) && >> - (iface_fsm[i].event == event)) { >> - new_state = iface_fsm[i].new_state; >> - break; >> - } >> - >> - if (iface_fsm[i].state == -1) { >> - /* event outside of the defined fsm, ignore it. */ >> - log_debug("if_fsm: interface %s, " >> - "event %s not expected in state %s", iface->name, >> - if_event_names[event], if_state_name(old_state)); >> - return (0); >> - } >> - >> - switch (iface_fsm[i].action) { >> - case IF_ACT_UPDATE: >> - ret = if_act_update(iface); >> - break; >> - case IF_ACT_RST: >> - ret = if_act_reset(iface); >> - break; >> - case IF_ACT_NOTHING: >> - /* do nothing */ >> - break; >> - } >> - >> - if (ret) { >> - log_debug("if_fsm: error changing state for interface %s, " >> - "event %s, state %s", iface->name, if_event_names[event], >> - if_state_name(old_state)); >> - return (-1); >> - } >> - >> - if (new_state != 0) >> - iface->state = new_state; >> - >> - log_debug("if_fsm: event %s resulted in action %s and changing " >> - "state for interface %s from %s to %s", >> - if_event_names[event], if_action_names[iface_fsm[i].action], >> - iface->name, if_state_name(old_state), >> if_state_name(iface->state)); >> - >> - return (ret); >> -} >> >> struct iface * >> if_new(struct kif *kif) >> @@ -168,7 +79,7 @@ if_del(struct iface *iface) >> struct if_addr *if_addr; >> >> if (iface->state == IF_STA_ACTIVE) >> - if_act_reset(iface); >> + if_reset(iface); >> >> log_debug("if_del: interface %s", iface->name); >> >> @@ -237,13 +148,14 @@ if_stop_hello_timer(struct iface *iface) >> fatal("if_stop_hello_timer"); >> } >> >> -/* actions */ >> int >> -if_act_start(struct iface *iface) >> +if_start(struct iface *iface) >> { >> struct in_addr addr; >> struct timeval now; >> >> + log_debug("if_start: %s", iface->name); >> + >> gettimeofday(&now, NULL); >> iface->uptime = now.tv_sec; >> >> @@ -257,11 +169,13 @@ if_act_start(struct iface *iface) >> } >> >> int >> -if_act_reset(struct iface *iface) >> +if_reset(struct iface *iface) >> { >> struct in_addr addr; >> struct adj *adj; >> >> + log_debug("if_reset: %s", iface->name); >> + >> while ((adj = LIST_FIRST(&iface->adj_list)) != NULL) { >> LIST_REMOVE(adj, iface_entry); >> adj_del(adj); >> @@ -277,26 +191,26 @@ if_act_reset(struct iface *iface) >> } >> >> int >> -if_act_update(struct iface *iface) >> +if_update(struct iface *iface) >> { >> int ret; >> >> if (iface->state == IF_STA_DOWN) { >> - if (!((iface->flags & IFF_UP) && >> - LINK_STATE_IS_UP(iface->linkstate))) >> - return (0); >> - >> - if (LIST_EMPTY(&iface->addr_list)) >> + if (!(iface->flags & IFF_UP) || >> + !LINK_STATE_IS_UP(iface->linkstate) || >> + LIST_EMPTY(&iface->addr_list)) >> return (0); >> >> iface->state = IF_STA_ACTIVE; >> - ret = if_act_start(iface); >> + ret = if_start(iface); >> } else { >> - if (!LIST_EMPTY(&iface->addr_list)) >> + if ((iface->flags & IFF_UP) && >> + LINK_STATE_IS_UP(iface->linkstate) && >> + !LIST_EMPTY(&iface->addr_list)) >> return (0); >> >> iface->state = IF_STA_DOWN; >> - ret = if_act_reset(iface); >> + ret = if_reset(iface); >> } >> >> return (ret); >> diff --git a/kroute.c b/kroute.c >> index 679f66d..a962ea1 100644 >> --- a/kroute.c >> +++ b/kroute.c >> @@ -136,7 +136,7 @@ kif_redistribute(void) >> struct kif_addr *ka; >> >> RB_FOREACH(kif, kif_tree, &kit) { >> - main_imsg_compose_ldpe(IMSG_IFUP, 0, &kif->k, >> + main_imsg_compose_ldpe(IMSG_IFSTATUS, 0, &kif->k, >> sizeof(struct kif)); >> >> TAILQ_FOREACH(ka, &kif->addrs, entry) >> @@ -855,18 +855,17 @@ if_change(u_short ifindex, int flags, struct if_data >> *ifd, >> link_new = (kif->k.flags & IFF_UP) && >> LINK_STATE_IS_UP(kif->k.link_state); >> >> - if (link_new == link_old) { >> - main_imsg_compose_ldpe(IMSG_IFSTATUS, 0, &kif->k, >> - sizeof(struct kif)); >> + if (link_new == link_old) >> return; >> - } else if (link_new) { >> - main_imsg_compose_ldpe(IMSG_IFUP, 0, &kif->k, >> + >> + if (link_new) { >> + main_imsg_compose_ldpe(IMSG_IFSTATUS, 0, &kif->k, >> sizeof(struct kif)); >> TAILQ_FOREACH(ka, &kif->addrs, entry) >> main_imsg_compose_ldpe(IMSG_NEWADDR, 0, &ka->addr, >> sizeof(struct kaddr)); >> } else { >> - main_imsg_compose_ldpe(IMSG_IFDOWN, 0, &kif->k, >> + main_imsg_compose_ldpe(IMSG_IFSTATUS, 0, &kif->k, >> sizeof(struct kif)); >> TAILQ_FOREACH(ka, &kif->addrs, entry) >> main_imsg_compose_ldpe(IMSG_DELADDR, 0, &ka->addr, >> diff --git a/ldpd.h b/ldpd.h >> index 27e7138..f3718dd 100644 >> --- a/ldpd.h >> +++ b/ldpd.h >> @@ -85,8 +85,6 @@ enum imsg_type { >> IMSG_KLABEL_CHANGE, >> IMSG_KLABEL_DELETE, >> IMSG_IFSTATUS, >> - IMSG_IFUP, >> - IMSG_IFDOWN, >> IMSG_NEWADDR, >> IMSG_DELADDR, >> IMSG_LABEL_MAPPING, >> @@ -117,23 +115,6 @@ enum imsg_type { >> #define IF_STA_NEW 0x00 /* dummy state for reload */ >> #define IF_STA_DOWN 0x01 >> #define IF_STA_ACTIVE 0x02 >> -#define IF_STA_ANY (IF_STA_DOWN | IF_STA_ACTIVE) >> - >> -/* interface events */ >> -enum iface_event { >> - IF_EVT_NOTHING, >> - IF_EVT_UP, >> - IF_EVT_DOWN, >> - IF_EVT_NEWADDR, >> - IF_EVT_DELADDR >> -}; >> - >> -/* interface actions */ >> -enum iface_action { >> - IF_ACT_NOTHING, >> - IF_ACT_UPDATE, >> - IF_ACT_RST >> -}; >> >> /* interface types */ >> enum iface_type { >> diff --git a/ldpe.c b/ldpe.c >> index d422603..9e2648f 100644 >> --- a/ldpe.c >> +++ b/ldpe.c >> @@ -264,10 +264,6 @@ ldpe_shutdown(void) >> >> /* stop all interfaces */ >> while ((iface = LIST_FIRST(&leconf->iface_list)) != NULL) { >> - if (if_fsm(iface, IF_EVT_DOWN)) { >> - log_debug("error stopping interface %s", >> - iface->name); >> - } >> LIST_REMOVE(iface, entry); >> if_del(iface); >> } >> @@ -346,8 +342,6 @@ ldpe_dispatch_main(int fd, short event, void *bula) >> >> switch (imsg.hdr.type) { >> case IMSG_IFSTATUS: >> - case IMSG_IFUP: >> - case IMSG_IFDOWN: >> if (imsg.hdr.len != IMSG_HEADER_SIZE + >> sizeof(struct kif)) >> fatalx("IFINFO imsg with wrong len"); >> @@ -359,16 +353,7 @@ ldpe_dispatch_main(int fd, short event, void *bula) >> >> iface->flags = kif->flags; >> iface->linkstate = kif->link_state; >> - switch (imsg.hdr.type) { >> - case IMSG_IFUP: >> - if_fsm(iface, IF_EVT_UP); >> - break; >> - case IMSG_IFDOWN: >> - if_fsm(iface, IF_EVT_DOWN); >> - break; >> - default: >> - break; >> - } >> + if_update(iface); >> break; >> case IMSG_NEWADDR: >> if (imsg.hdr.len != IMSG_HEADER_SIZE + >> @@ -395,7 +380,7 @@ ldpe_dispatch_main(int fd, short event, void *bula) >> if (iface) { >> LIST_INSERT_HEAD(&iface->addr_list, if_addr, >> iface_entry); >> - if_fsm(iface, IF_EVT_NEWADDR); >> + if_update(iface); >> } >> break; >> case IMSG_DELADDR: >> @@ -417,7 +402,7 @@ ldpe_dispatch_main(int fd, short event, void *bula) >> iface = if_lookup(kaddr->ifindex); >> if (iface) { >> LIST_REMOVE(if_addr, iface_entry); >> - if_fsm(iface, IF_EVT_DELADDR); >> + if_update(iface); >> } >> >> RB_FOREACH(nbr, nbr_id_head, &nbrs_by_id) { >> diff --git a/ldpe.h b/ldpe.h >> index ffb5848..e380d65 100644 >> --- a/ldpe.h >> +++ b/ldpe.h >> @@ -136,7 +136,9 @@ void ldpe_fib_update(int); >> void ldpe_iface_ctl(struct ctl_conn *, unsigned int); >> >> /* interface.c */ >> -int if_fsm(struct iface *, enum iface_event); >> +int if_start(struct iface *); >> +int if_reset(struct iface *); >> +int if_update(struct iface *); >> >> struct iface *if_new(struct kif *); >> void if_del(struct iface *); >> -- >> 1.9.1 >> > > Looks good to me. Wonder if we lose any important log message because of > this. Anyway, now is the rigth time to move so OK claudio@
No need to worry, I've introduced two debug messages in if_start() and if_reset() to track when an interface is enabled or disabled respectively. I think that all the previous interface FSM transitions were too verbose for little use, for me it looks better now. -- Renato Westphal
