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@
--
:wq Claudio
