diff --git a/bgpd/bgp_debug.c b/bgpd/bgp_debug.c index 1b398ee..090d513 100644 --- a/bgpd/bgp_debug.c +++ b/bgpd/bgp_debug.c @@ -62,6 +62,8 @@ struct message bgp_status_msg[] = { OpenSent, "OpenSent" }, { OpenConfirm, "OpenConfirm" }, { Established, "Established" }, + { Clearing, "Clearing" }, + { Deleted, "Deleted" }, }; int bgp_status_msg_max = BGP_STATUS_MAX; diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c index 770a791..2c57269 100644 --- a/bgpd/bgp_fsm.c +++ b/bgpd/bgp_fsm.c @@ -68,6 +68,11 @@ bgp_start_jitter (int time) return ((rand () % (time + 1)) - (time / 2)); } +/* Check if suppress start/restart of sessions to peer. */ +#define BGP_PEER_START_SUPPRESSED(P) \ + (CHECK_FLAG ((P)->flags, PEER_FLAG_SHUTDOWN) \ + || CHECK_FLAG ((P)->sflags, PEER_STATUS_PREFIX_OVERFLOW)) + /* Hook function called after bgp event is occered. And vty's neighbor command invoke this function after making neighbor structure. */ @@ -82,10 +87,7 @@ bgp_timer_set (struct peer *peer) /* First entry point of peer's finite state machine. In Idle status start timer is on unless peer is shutdown or peer is inactive. All other timer must be turned off */ - if (CHECK_FLAG (peer->flags, PEER_FLAG_SHUTDOWN) - || CHECK_FLAG (peer->sflags, PEER_STATUS_PREFIX_OVERFLOW) - || CHECK_FLAG (peer->sflags, PEER_STATUS_CLEARING) - || ! peer_active (peer)) + if (BGP_PEER_START_SUPPRESSED(peer) || ! peer_active (peer)) { BGP_TIMER_OFF (peer->t_start); } @@ -197,6 +199,18 @@ bgp_timer_set (struct peer *peer) } BGP_TIMER_OFF (peer->t_asorig); break; + case Deleted: + BGP_TIMER_OFF (peer->t_gr_restart); + BGP_TIMER_OFF (peer->t_gr_stale); + BGP_TIMER_OFF (peer->t_pmax_restart); + case Clearing: + BGP_TIMER_OFF (peer->t_start); + BGP_TIMER_OFF (peer->t_connect); + BGP_TIMER_OFF (peer->t_holdtime); + BGP_TIMER_OFF (peer->t_keepalive); + BGP_TIMER_OFF (peer->t_asorig); + BGP_TIMER_OFF (peer->t_routeadv); + break; } } @@ -413,14 +427,12 @@ bgp_stop (struct peer *peer) { afi_t afi; safi_t safi; - unsigned int i; char orf_name[BUFSIZ]; /* Increment Dropped count. */ if (peer->status == Established) { peer->dropped++; - bgp_fsm_change_status (peer, Idle); /* bgp log-neighbor-changes of neighbor Down */ if (bgp_flag_check (peer->bgp, BGP_FLAG_LOG_NEIGHBOR_CHANGES)) @@ -490,8 +502,7 @@ #endif /* HAVE_SNMP */ /* Delete all existing events of the peer, and corresponding peer ref-count */ - for (i = thread_cancel_event (master, peer); i > 0; i--) - peer_unlock (peer); /* thread event reference */ + BGP_EVENT_FLUSH (peer); /* Stream reset. */ peer->packet_size = 0; @@ -624,7 +635,14 @@ int bgp_start (struct peer *peer) { int status; - + + if (BGP_PEER_START_SUPPRESSED (peer)) + { + if (BGP_DEBUG (fsm, FSM)) + plog_err (peer->log, "%s [FSM] Trying to start suppressed peer" + " - this is never supposed to happen!", peer->host); + return -1; + } /* Scrub some information that might be left over from a previous, * session */ @@ -764,7 +782,6 @@ bgp_establish (struct peer *peer) /* Increment established count. */ peer->established++; - bgp_fsm_change_status (peer, Established); /* bgp log-neighbor-changes of neighbor Up */ if (bgp_flag_check (peer->bgp, BGP_FLAG_LOG_NEIGHBOR_CHANGES)) @@ -873,10 +890,11 @@ bgp_fsm_update (struct peer *peer) /* This is empty event. */ static int -bgp_ignore (struct peer *peer) +bgp_impossible (struct peer *peer) { if (BGP_DEBUG (fsm, FSM)) - zlog (peer->log, LOG_DEBUG, "%s [FSM] bgp_ignore called", peer->host); + plog_warn (peer->log, "%s [FSM] Eek, impossible event for state %s", + peer->host, LOOKUP (bgp_status_msg, peer->status)); return 0; } @@ -890,99 +908,147 @@ struct { /* Idle state: In Idle state, all events other than BGP_Start is ignored. With BGP_Start event, finite state machine calls bgp_start(). */ - {bgp_start, Connect}, /* BGP_Start */ - {bgp_stop, Idle}, /* BGP_Stop */ - {bgp_stop, Idle}, /* TCP_connection_open */ - {bgp_stop, Idle}, /* TCP_connection_closed */ - {bgp_ignore, Idle}, /* TCP_connection_open_failed */ - {bgp_stop, Idle}, /* TCP_fatal_error */ - {bgp_ignore, Idle}, /* ConnectRetry_timer_expired */ - {bgp_ignore, Idle}, /* Hold_Timer_expired */ - {bgp_ignore, Idle}, /* KeepAlive_timer_expired */ - {bgp_ignore, Idle}, /* Receive_OPEN_message */ - {bgp_ignore, Idle}, /* Receive_KEEPALIVE_message */ - {bgp_ignore, Idle}, /* Receive_UPDATE_message */ - {bgp_ignore, Idle}, /* Receive_NOTIFICATION_message */ + {bgp_start, Connect}, /* BGP_Start */ + {bgp_stop, Idle}, /* BGP_Stop */ + {bgp_stop, Idle}, /* TCP_connection_open */ + {bgp_stop, Idle}, /* TCP_connection_closed */ + {NULL, Idle}, /* TCP_connection_open_failed */ + {bgp_stop, Idle}, /* TCP_fatal_error */ + {bgp_impossible, Idle}, /* ConnectRetry_timer_expired */ + {bgp_impossible, Idle}, /* Hold_Timer_expired */ + {bgp_impossible, Idle}, /* KeepAlive_timer_expired */ + {bgp_impossible, Idle}, /* Receive_OPEN_message */ + {bgp_impossible, Idle}, /* Receive_KEEPALIVE_message */ + {bgp_impossible, Idle}, /* Receive_UPDATE_message */ + {bgp_impossible, Idle}, /* Receive_NOTIFICATION_message */ + {NULL, Idle}, /* Clearing_Completed */ + {bgp_impossible, Idle}, /* Prefix_Overflowed */ }, { /* Connect */ - {bgp_ignore, Connect}, /* BGP_Start */ - {bgp_stop, Idle}, /* BGP_Stop */ - {bgp_connect_success, OpenSent}, /* TCP_connection_open */ - {bgp_stop, Idle}, /* TCP_connection_closed */ - {bgp_connect_fail, Active}, /* TCP_connection_open_failed */ - {bgp_connect_fail, Idle}, /* TCP_fatal_error */ - {bgp_reconnect, Connect}, /* ConnectRetry_timer_expired */ - {bgp_ignore, Idle}, /* Hold_Timer_expired */ - {bgp_ignore, Idle}, /* KeepAlive_timer_expired */ - {bgp_ignore, Idle}, /* Receive_OPEN_message */ - {bgp_ignore, Idle}, /* Receive_KEEPALIVE_message */ - {bgp_ignore, Idle}, /* Receive_UPDATE_message */ - {bgp_stop, Idle}, /* Receive_NOTIFICATION_message */ + {NULL, Connect }, /* BGP_Start */ + {bgp_stop, Clearing}, /* BGP_Stop */ + {bgp_connect_success, OpenSent}, /* TCP_connection_open */ + {bgp_stop, Clearing}, /* TCP_connection_closed */ + {bgp_connect_fail, Active}, /* TCP_connection_open_failed */ + {bgp_connect_fail, Idle}, /* TCP_fatal_error */ + {bgp_reconnect, Connect}, /* ConnectRetry_timer_expired */ + {bgp_impossible, Idle}, /* Hold_Timer_expired */ + {bgp_impossible, Idle}, /* KeepAlive_timer_expired */ + {bgp_impossible, Idle}, /* Receive_OPEN_message */ + {bgp_impossible, Idle}, /* Receive_KEEPALIVE_message */ + {bgp_impossible, Idle}, /* Receive_UPDATE_message */ + {bgp_stop, Idle}, /* Receive_NOTIFICATION_message */ + {bgp_impossible, Idle}, /* Clearing_Completed */ + {bgp_impossible, Idle}, /* Prefix_Overflowed */ }, { /* Active, */ - {bgp_ignore, Active}, /* BGP_Start */ - {bgp_stop, Idle}, /* BGP_Stop */ - {bgp_connect_success, OpenSent}, /* TCP_connection_open */ - {bgp_stop, Idle}, /* TCP_connection_closed */ - {bgp_ignore, Active}, /* TCP_connection_open_failed */ - {bgp_ignore, Idle}, /* TCP_fatal_error */ - {bgp_start, Connect}, /* ConnectRetry_timer_expired */ - {bgp_ignore, Idle}, /* Hold_Timer_expired */ - {bgp_ignore, Idle}, /* KeepAlive_timer_expired */ - {bgp_ignore, Idle}, /* Receive_OPEN_message */ - {bgp_ignore, Idle}, /* Receive_KEEPALIVE_message */ - {bgp_ignore, Idle}, /* Receive_UPDATE_message */ - {bgp_stop_with_error, Idle}, /* Receive_NOTIFICATION_message */ + {NULL, Active}, /* BGP_Start */ + {bgp_stop, Idle}, /* BGP_Stop */ + {bgp_connect_success, OpenSent}, /* TCP_connection_open */ + {bgp_stop, Idle}, /* TCP_connection_closed */ + {NULL, Active}, /* TCP_connection_open_failed */ + {NULL, Idle}, /* TCP_fatal_error */ + {bgp_start, Connect}, /* ConnectRetry_timer_expired */ + {bgp_impossible, Idle}, /* Hold_Timer_expired */ + {bgp_impossible, Idle}, /* KeepAlive_timer_expired */ + {NULL, Idle}, /* Receive_OPEN_message */ + {bgp_impossible, Idle}, /* Receive_KEEPALIVE_message */ + {bgp_impossible, Idle}, /* Receive_UPDATE_message */ + {bgp_stop_with_error, Idle}, /* Receive_NOTIFICATION_message */ + {bgp_impossible, Idle}, /* Clearing_Completed */ + {bgp_impossible, Idle}, /* Prefix_Overflowed */ }, { /* OpenSent, */ - {bgp_ignore, OpenSent}, /* BGP_Start */ - {bgp_stop, Idle}, /* BGP_Stop */ - {bgp_stop, Idle}, /* TCP_connection_open */ - {bgp_stop, Active}, /* TCP_connection_closed */ - {bgp_ignore, Idle}, /* TCP_connection_open_failed */ - {bgp_stop, Idle}, /* TCP_fatal_error */ - {bgp_ignore, Idle}, /* ConnectRetry_timer_expired */ + {NULL, OpenSent}, /* BGP_Start */ + {bgp_stop, Idle}, /* BGP_Stop */ + {bgp_stop, Idle}, /* TCP_connection_open */ + {bgp_stop, Active}, /* TCP_connection_closed */ + {NULL, Idle}, /* TCP_connection_open_failed */ + {bgp_stop, Idle}, /* TCP_fatal_error */ + {NULL, Idle}, /* ConnectRetry_timer_expired */ {bgp_fsm_holdtime_expire, Idle}, /* Hold_Timer_expired */ - {bgp_ignore, Idle}, /* KeepAlive_timer_expired */ - {bgp_fsm_open, OpenConfirm}, /* Receive_OPEN_message */ - {bgp_ignore, Idle}, /* Receive_KEEPALIVE_message */ - {bgp_ignore, Idle}, /* Receive_UPDATE_message */ - {bgp_stop_with_error, Idle}, /* Receive_NOTIFICATION_message */ + {NULL, Idle}, /* KeepAlive_timer_expired */ + {bgp_fsm_open, OpenConfirm}, /* Receive_OPEN_message */ + {bgp_impossible, Idle}, /* Receive_KEEPALIVE_message */ + {bgp_impossible, Idle}, /* Receive_UPDATE_message */ + {bgp_stop_with_error, Idle}, /* Receive_NOTIFICATION_message */ + {bgp_impossible, Idle}, /* Clearing_Completed */ + {bgp_impossible, Idle}, /* Prefix_Overflowed */ }, { /* OpenConfirm, */ - {bgp_ignore, OpenConfirm}, /* BGP_Start */ - {bgp_stop, Idle}, /* BGP_Stop */ - {bgp_stop, Idle}, /* TCP_connection_open */ - {bgp_stop, Idle}, /* TCP_connection_closed */ - {bgp_stop, Idle}, /* TCP_connection_open_failed */ - {bgp_stop, Idle}, /* TCP_fatal_error */ - {bgp_ignore, Idle}, /* ConnectRetry_timer_expired */ + {NULL, OpenConfirm}, /* BGP_Start */ + {bgp_stop, Idle}, /* BGP_Stop */ + {bgp_stop, Idle}, /* TCP_connection_open */ + {bgp_stop, Idle}, /* TCP_connection_closed */ + {bgp_stop, Idle}, /* TCP_connection_open_failed */ + {bgp_stop, Idle}, /* TCP_fatal_error */ + {NULL, Idle}, /* ConnectRetry_timer_expired */ {bgp_fsm_holdtime_expire, Idle}, /* Hold_Timer_expired */ - {bgp_ignore, OpenConfirm}, /* KeepAlive_timer_expired */ - {bgp_ignore, Idle}, /* Receive_OPEN_message */ - {bgp_establish, Established}, /* Receive_KEEPALIVE_message */ - {bgp_ignore, Idle}, /* Receive_UPDATE_message */ - {bgp_stop_with_error, Idle}, /* Receive_NOTIFICATION_message */ + {NULL, OpenConfirm}, /* KeepAlive_timer_expired */ + {NULL, Idle}, /* Receive_OPEN_message */ + {bgp_establish, Established}, /* Receive_KEEPALIVE_message */ + {bgp_impossible, Idle}, /* Receive_UPDATE_message */ + {bgp_stop_with_error, Idle}, /* Receive_NOTIFICATION_message */ + {bgp_impossible, Idle}, /* Clearing_Completed */ + {bgp_impossible, Idle}, /* Prefix_Overflowed */ }, { /* Established, */ - {bgp_ignore, Established}, /* BGP_Start */ - {bgp_stop, Idle}, /* BGP_Stop */ - {bgp_stop, Idle}, /* TCP_connection_open */ - {bgp_stop, Idle}, /* TCP_connection_closed */ - {bgp_ignore, Idle}, /* TCP_connection_open_failed */ - {bgp_stop, Idle}, /* TCP_fatal_error */ - {bgp_ignore, Idle}, /* ConnectRetry_timer_expired */ - {bgp_fsm_holdtime_expire, Idle}, /* Hold_Timer_expired */ + {NULL, Established}, /* BGP_Start */ + {bgp_stop, Clearing}, /* BGP_Stop */ + {bgp_stop, Clearing}, /* TCP_connection_open */ + {bgp_stop, Clearing}, /* TCP_connection_closed */ + {NULL, Clearing}, /* TCP_connection_open_failed */ + {bgp_stop, Clearing}, /* TCP_fatal_error */ + {bgp_impossible, Clearing}, /* ConnectRetry_timer_expired */ + {bgp_fsm_holdtime_expire, Clearing}, /* Hold_Timer_expired */ {bgp_fsm_keepalive_expire, Established}, /* KeepAlive_timer_expired */ - {bgp_stop, Idle}, /* Receive_OPEN_message */ - {bgp_fsm_keepalive, Established}, /* Receive_KEEPALIVE_message */ - {bgp_fsm_update, Established}, /* Receive_UPDATE_message */ - {bgp_stop_with_error, Idle}, /* Receive_NOTIFICATION_message */ + {bgp_stop, Clearing}, /* Receive_OPEN_message */ + {bgp_fsm_keepalive, Established}, /* Receive_KEEPALIVE_message */ + {bgp_fsm_update, Established}, /* Receive_UPDATE_message */ + {bgp_stop_with_error, Clearing}, /* Receive_NOTIFICATION_message */ + {bgp_impossible, Idle}, /* Clearing_Completed */ + {bgp_stop, Clearing}, /* Prefix_Overflowed */ + }, + { + /* Clearing, */ + {NULL, Clearing}, /* BGP_Start */ + {NULL, Clearing}, /* BGP_Stop */ + {bgp_impossible, Clearing}, /* TCP_connection_open */ + {bgp_impossible, Clearing}, /* TCP_connection_closed */ + {bgp_impossible, Clearing}, /* TCP_connection_open_failed */ + {bgp_impossible, Clearing}, /* TCP_fatal_error */ + {bgp_impossible, Clearing}, /* ConnectRetry_timer_expired */ + {bgp_impossible, Clearing}, /* Hold_Timer_expired */ + {bgp_impossible, Clearing}, /* KeepAlive_timer_expired */ + {bgp_impossible, Clearing}, /* Receive_OPEN_message */ + {bgp_impossible, Clearing}, /* Receive_KEEPALIVE_message */ + {bgp_impossible, Clearing}, /* Receive_UPDATE_message */ + {NULL, Clearing}, /* Receive_NOTIFICATION_message */ + {NULL, Idle}, /* Clearing_Completed */ + {NULL, Clearing}, /* Prefix_Overflowed */ + }, + { + /* Deleted, */ + {bgp_impossible, Deleted}, /* BGP_Start */ + {NULL, Deleted}, /* BGP_Stop */ + {bgp_impossible, Deleted}, /* TCP_connection_open */ + {bgp_impossible, Deleted}, /* TCP_connection_closed */ + {bgp_impossible, Deleted}, /* TCP_connection_open_failed */ + {bgp_impossible, Deleted}, /* TCP_fatal_error */ + {bgp_impossible, Deleted}, /* ConnectRetry_timer_expired */ + {bgp_impossible, Deleted}, /* Hold_Timer_expired */ + {bgp_impossible, Deleted}, /* KeepAlive_timer_expired */ + {bgp_impossible, Deleted}, /* Receive_OPEN_message */ + {bgp_impossible, Deleted}, /* Receive_KEEPALIVE_message */ + {bgp_impossible, Deleted}, /* Receive_UPDATE_message */ + {bgp_impossible, Deleted}, /* Receive_NOTIFICATION_message */ + {NULL, Deleted}, /* Clearing_Completed */ + {bgp_impossible, Deleted}, /* Prefix_Overflowed */ }, }; @@ -1001,14 +1067,16 @@ static const char *bgp_event_str[] = "Receive_OPEN_message", "Receive_KEEPALIVE_message", "Receive_UPDATE_message", - "Receive_NOTIFICATION_message" + "Receive_NOTIFICATION_message", + "Clearing_Completed", + "Prefix_Overflowed", }; /* Execute event process. */ int bgp_event (struct thread *thread) { - int ret; + int ret = 0; int event; int next; struct peer *peer; @@ -1019,14 +1087,15 @@ bgp_event (struct thread *thread) /* Logging this event. */ next = FSM [peer->status -1][event - 1].next_state; - if (BGP_DEBUG (fsm, FSM)) + if (BGP_DEBUG (fsm, FSM) && peer->status != next) plog_debug (peer->log, "%s [FSM] %s (%s->%s)", peer->host, bgp_event_str[event], LOOKUP (bgp_status_msg, peer->status), LOOKUP (bgp_status_msg, next)); /* Call function. */ - ret = (*(FSM [peer->status - 1][event - 1].func))(peer); + if (FSM [peer->status -1][event - 1].func) + ret = (*(FSM [peer->status - 1][event - 1].func))(peer); /* When function do not want proceed next job return -1. */ if (ret >= 0) @@ -1039,6 +1108,5 @@ bgp_event (struct thread *thread) bgp_timer_set (peer); } - peer_unlock (peer); /* bgp-event peer reference */ return ret; } diff --git a/bgpd/bgp_fsm.h b/bgpd/bgp_fsm.h index e90f3b4..dbe90f8 100644 --- a/bgpd/bgp_fsm.h +++ b/bgpd/bgp_fsm.h @@ -22,74 +22,72 @@ Software Foundation, Inc., 59 Temple Pla #ifndef _QUAGGA_BGP_FSM_H #define _QUAGGA_BGP_FSM_H +#include "lib/log.h" +#include "bgp_debug.h" + /* Macro for BGP read, write and timer thread. */ +#define BGP_THREAD_DEBUG(S) \ + do { \ + if (peer->lock < 10) \ + zlog_debug ("%s/%s: %p (%s:%s) lock: %d", \ + __func__, (S), peer, peer->host, \ + LOOKUP (bgp_status_msg, peer->status), \ + peer->lock); \ + } while (0) + #define BGP_READ_ON(T,F,V) \ do { \ - if (!T) \ - { \ - peer_lock (peer); \ - THREAD_READ_ON(master,T,F,peer,V); \ - } \ + if (!(T) && (peer->status != Deleted)) \ + THREAD_READ_ON(master,T,F,peer,V); \ } while (0) #define BGP_READ_OFF(T) \ do { \ if (T) \ - { \ - peer_unlock (peer); \ THREAD_READ_OFF(T); \ - } \ } while (0) #define BGP_WRITE_ON(T,F,V) \ do { \ - if (!T) \ - { \ - peer_lock (peer); \ + if (!(T) && peer->status != Deleted) \ THREAD_WRITE_ON(master,(T),(F),peer,(V)); \ - } \ } while (0) #define BGP_WRITE_OFF(T) \ do { \ if (T) \ - { \ - peer_unlock (peer); \ THREAD_WRITE_OFF(T); \ - } \ } while (0) #define BGP_TIMER_ON(T,F,V) \ do { \ - if (!T) \ - { \ - peer_lock (peer); \ + if (!(T) && peer->status != Deleted) \ THREAD_TIMER_ON(master,(T),(F),peer,(V)); \ - } \ } while (0) #define BGP_TIMER_OFF(T) \ do { \ - if (T) \ - { \ - peer_unlock (peer); \ + if (T) \ THREAD_TIMER_OFF(T); \ - } \ } while (0) -#define BGP_EVENT_ADD(P,E) \ - do { \ - peer_lock (peer); /* bgp event reference */ \ - thread_add_event (master, bgp_event, (P), (E)); \ +#define BGP_EVENT_ADD(P,E) \ + do { \ + if ((P)->status != Deleted) \ + thread_add_event (master, bgp_event, (P), (E)); \ } while (0) -#define BGP_EVENT_DELETE(P) \ - do { \ - peer_unlock (peer); /* bgp event peer reference */ \ - assert (peer); \ - thread_cancel_event (master, (P)); \ +#define BGP_EVENT_FLUSH(P) \ + do { \ + thread_cancel_event (master, (P)); \ } while (0) +#define BGP_EVENT_FLUSH_ADD(P,E) \ + do { \ + BGP_EVENT_FLUSH(P); \ + BGP_EVENT_ADD(P,E); \ + } while (0); + /* Prototypes. */ extern int bgp_event (struct thread *); extern int bgp_stop (struct peer *peer); diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index 8b024a1..0fc5569 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -636,10 +636,8 @@ bgp_write (struct thread *thread) /* Partial write. */ if (write_errno == EWOULDBLOCK || write_errno == EAGAIN) break; - - BGP_EVENT_ADD (peer, BGP_Stop); - peer->status = Idle; - bgp_timer_set (peer); + + BGP_EVENT_FLUSH_ADD (peer, TCP_fatal_error); return 0; } if (num != writenum) @@ -673,10 +671,8 @@ bgp_write (struct thread *thread) if (peer->v_start >= (60 * 2)) peer->v_start = (60 * 2); - BGP_EVENT_ADD (peer, BGP_Stop); - /*bgp_stop (peer);*/ - peer->status = Idle; - bgp_timer_set (peer); + /* Flush any existing events */ + BGP_EVENT_FLUSH_ADD (peer, BGP_Stop); return 0; case BGP_MSG_KEEPALIVE: peer->keepalive_out++; @@ -721,9 +717,7 @@ bgp_write_notify (struct peer *peer) ret = writen (peer->fd, STREAM_DATA (s), stream_get_endp (s)); if (ret <= 0) { - BGP_EVENT_ADD (peer, BGP_Stop); - peer->status = Idle; - bgp_timer_set (peer); + BGP_EVENT_FLUSH_ADD (peer, TCP_fatal_error); return 0; } @@ -743,10 +737,7 @@ bgp_write_notify (struct peer *peer) if (peer->v_start >= (60 * 2)) peer->v_start = (60 * 2); - /* We don't call event manager at here for avoiding other events. */ - bgp_stop (peer); - peer->status = Idle; - bgp_timer_set (peer); + BGP_EVENT_FLUSH_ADD (peer, BGP_Stop); return 0; } @@ -2373,16 +2364,10 @@ bgp_read (struct thread *thread) if (CHECK_FLAG (peer->sflags, PEER_STATUS_ACCEPT_PEER)) { if (BGP_DEBUG (events, EVENTS)) - zlog_debug ("%s [Event] Accepting BGP peer delete", peer->host); + zlog_debug ("%s [Event] Accepting BGP peer delete %d", peer->host, + peer->lock); peer_delete (peer); - /* we've lost track of a reference to ACCEPT_PEER somehow. It doesnt - * _seem_ to be the 'update realpeer with accept peer' hack, yet it - * *must* be.. Very very odd, but I give up trying to - * root cause this - ACCEPT_PEER is a dirty hack, it should be fixed - * instead, which would make root-causing this a moot point.. - * A hack because of a hack, appropriate. - */ - peer_unlock (peer); /* god knows what reference... ACCEPT_PEER sucks */ } + return 0; } diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 5623cad..2b2942c 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -1413,7 +1413,7 @@ bgp_process_queue_init (void) bm->process_main_queue->spec.max_retries = bm->process_main_queue->spec.max_retries = 0; bm->process_rsclient_queue->spec.hold - = bm->process_main_queue->spec.hold = 500; + = bm->process_main_queue->spec.hold = 50; } void @@ -1522,6 +1522,8 @@ bgp_maximum_prefix_overflow (struct peer BGP_TIMER_ON (peer->t_pmax_restart, bgp_maximum_prefix_restart_timer, peer->v_pmax_restart); } + + BGP_EVENT_ADD (peer, Prefix_Overflowed); return 1; } @@ -2516,11 +2518,10 @@ bgp_clear_node_complete (struct work_que { struct peer *peer = wq->spec.data; - UNSET_FLAG (peer->sflags, PEER_STATUS_CLEARING); peer_unlock (peer); /* bgp_clear_node_complete */ - /* Tickle FSM to start moving again */ - BGP_EVENT_ADD (peer, BGP_Start); + /* Tickle FSM */ + BGP_EVENT_ADD (peer, Clearing_Completed); } static void @@ -2582,9 +2583,10 @@ bgp_clear_route (struct peer *peer, afi_ if (peer->clear_node_queue == NULL) bgp_clear_node_queue_init (peer); - /* bgp_fsm.c will not bring CLEARING sessions out of Idle this - * protects against peers which flap faster than we can we clear, - * which could lead to: + /* bgp_fsm.c keeps sessions in state Clearing, not transitioning to + * Idle until it receives a Clearing_Completed event. This protects + * against peers which flap faster than we can we clear, which could + * lead to: * * a) race with routes from the new session being installed before * clear_route_node visits the node (to delete the route of that @@ -2593,11 +2595,8 @@ bgp_clear_route (struct peer *peer, afi_ * on the process_main queue. Fast-flapping could cause that queue * to grow and grow. */ - if (!CHECK_FLAG (peer->sflags, PEER_STATUS_CLEARING)) - { - SET_FLAG (peer->sflags, PEER_STATUS_CLEARING); - peer_lock (peer); /* bgp_clear_node_complete */ - } + if (!peer->clear_node_queue->thread) + peer_lock (peer); /* bgp_clear_node_complete */ if (safi != SAFI_MPLS_VPN) bgp_clear_route_table (peer, afi, safi, NULL, NULL); @@ -2613,11 +2612,37 @@ bgp_clear_route (struct peer *peer, afi_ bgp_clear_route_table (peer, afi, safi, NULL, rsclient); } - /* If no routes were cleared, nothing was added to workqueue, run the - * completion function now. + /* If no routes were cleared, nothing was added to workqueue, the + * completion function won't be run by workqueue code - call it here. + * + * Additionally, there is a presumption in FSM that clearing is only + * needed if peer state is Established - peers in pre-Established states + * shouldn't have any route-update state associated with them (in or out). + * + * We still get here from FSM through bgp_stop->clear_route_all in + * pre-Established though, so this is a useful sanity check to ensure + * the assumption above holds. + * + * At some future point, this check could be move to the top of the + * function, and do a quick early-return when state is + * pre-Established, avoiding above list and table scans. Once we're + * sure it is safe.. */ if (!peer->clear_node_queue->thread) - bgp_clear_node_complete (peer->clear_node_queue); + bgp_clear_node_complete (peer->clear_node_queue); + else + { + /* clearing queue scheduled. Normal if in Established state + * (and about to transition out of it), but otherwise... + */ + if (peer->status != Established) + { + plog_err (peer->log, "%s [Error] State %s is not Established," + " but routes were cleared - bug!", + peer->host, LOOKUP (bgp_status_msg, peer->status)); + assert (peer->status == Established); + } + } } void diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index ec4b6c2..b108164 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -6693,8 +6693,6 @@ bgp_show_summary (struct vty *vty, struc vty_out (vty, " Idle (Admin)"); else if (CHECK_FLAG (peer->sflags, PEER_STATUS_PREFIX_OVERFLOW)) vty_out (vty, " Idle (PfxCt)"); - else if (CHECK_FLAG (peer->sflags, PEER_STATUS_CLEARING)) - vty_out (vty, " Idle (Clrng)"); else vty_out (vty, " %-11s", LOOKUP(bgp_status_msg, peer->status)); } diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 8ed598d..d26b64a 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -687,6 +687,16 @@ peer_sort (struct peer *peer) static inline void peer_free (struct peer *peer) { + assert (peer->status == Deleted); + + /* this /ought/ to have been done already through bgp_stop earlier, + * but just to be sure.. + */ + bgp_timer_set (peer); + BGP_READ_OFF (peer->t_read); + BGP_WRITE_OFF (peer->t_write); + BGP_EVENT_FLUSH (peer); + if (peer->desc) XFREE (MTYPE_PEER_DESC, peer->desc); @@ -704,7 +714,9 @@ peer_free (struct peer *peer) if (peer->clear_node_queue) work_queue_free (peer->clear_node_queue); - memset (peer, 0, sizeof (struct peer)); + bgp_sync_delete (peer); + + memset (peer, 0x0000dead, sizeof (struct peer)); XFREE (MTYPE_BGP_PEER, peer); } @@ -715,8 +727,22 @@ peer_lock (struct peer *peer) { assert (peer && (peer->lock >= 0)); + assert (peer->status != Deleted); + peer->lock++; - + +#if 1 + if (CHECK_FLAG (peer->sflags, PEER_STATUS_ACCEPT_PEER) + || peer->status == Deleted + || (peer->lock < 10)) + { + zlog_debug ("%s: %p (%s:%s) lock: %d", + __func__, peer, (peer->host ? peer->host : "null"), + LOOKUP (bgp_status_msg, peer->status), peer->lock); + //zlog_backtrace (LOG_DEBUG); + } +#endif + return peer; } @@ -729,24 +755,26 @@ peer_unlock (struct peer *peer) assert (peer && (peer->lock > 0)); peer->lock--; - - if (peer->lock == 0) + +#if 1 + if (CHECK_FLAG (peer->sflags, PEER_STATUS_ACCEPT_PEER) + || (peer->status == Deleted) + || (peer->lock < 10)) { -#if 0 - zlog_debug ("unlocked and freeing"); - zlog_backtrace (LOG_DEBUG); -#endif - peer_free (peer); - return NULL; + zlog_debug ("%s: %p (%s:%s) lock: %d", + __func__, peer, (peer->host ? peer->host : "null"), + LOOKUP (bgp_status_msg, peer->status), peer->lock); + //zlog_backtrace (LOG_DEBUG); } +#endif -#if 0 - if (peer->lock == 1) + if (peer->lock == 0) { - zlog_debug ("unlocked to 1"); + assert (peer->status == Deleted); zlog_backtrace (LOG_DEBUG); + peer_free (peer); + return NULL; } -#endif return peer; } @@ -761,17 +789,16 @@ peer_new () struct servent *sp; /* Allocate new peer. */ - peer = XMALLOC (MTYPE_BGP_PEER, sizeof (struct peer)); - memset (peer, 0, sizeof (struct peer)); + peer = XCALLOC (MTYPE_BGP_PEER, sizeof (struct peer)); /* Set default value. */ peer->fd = -1; - peer->lock = 1; peer->v_start = BGP_INIT_START_TIMER; peer->v_connect = BGP_DEFAULT_CONNECT_RETRY; peer->v_asorig = BGP_DEFAULT_ASORIGINATE; peer->status = Idle; peer->ostatus = Idle; + peer = peer_lock (peer); /* initial reference */ peer->weight = 0; /* Set default flags. */ @@ -1139,7 +1166,17 @@ peer_nsf_stop (struct peer *peer) bgp_clear_route_all (peer); } -/* Delete peer from confguration. */ +/* Delete peer from configuration. + * + * The peer is moved to a dead-end "Deleted" neighbour-state, to allow + * it to "cool off" and refcounts to hit 0, at which state it is freed. + * + * This function /should/ take care to be idempotent, to guard against + * it being called multiple times through stray events that come in + * that happen to result in this function being called again. That + * said, getting here for a "Deleted" peer is a bug in the neighbour + * FSM. + */ int peer_delete (struct peer *peer) { @@ -1148,7 +1185,9 @@ peer_delete (struct peer *peer) safi_t safi; struct bgp *bgp; struct bgp_filter *filter; - + + assert (peer->status != Deleted); + bgp = peer->bgp; if (CHECK_FLAG (peer->sflags, PEER_STATUS_NSF_WAIT)) @@ -1158,8 +1197,13 @@ peer_delete (struct peer *peer) relationship. */ if (peer->group) { - peer = peer_unlock (peer); /* peer-group reference */ - listnode_delete (peer->group->peer, peer); + struct listnode *pn; + + if ((pn = listnode_lookup (peer->group->peer, peer))) + { + peer = peer_unlock (peer); /* group->peer list reference */ + list_delete_node (peer->group->peer, pn); + } peer->group = NULL; } @@ -1169,29 +1213,25 @@ peer_delete (struct peer *peer) */ peer->last_reset = PEER_DOWN_NEIGHBOR_DELETE; bgp_stop (peer); - bgp_fsm_change_status (peer, Idle); /* stops all timers */ + bgp_fsm_change_status (peer, Deleted); + bgp_timer_set (peer); /* stops all timers for Deleted */ - /* Stop all timers - should already have been done in bgp_stop */ - BGP_TIMER_OFF (peer->t_start); - BGP_TIMER_OFF (peer->t_connect); - BGP_TIMER_OFF (peer->t_holdtime); - BGP_TIMER_OFF (peer->t_keepalive); - BGP_TIMER_OFF (peer->t_asorig); - BGP_TIMER_OFF (peer->t_routeadv); - BGP_TIMER_OFF (peer->t_pmax_restart); - BGP_TIMER_OFF (peer->t_gr_restart); - BGP_TIMER_OFF (peer->t_gr_stale); - /* Delete from all peer list. */ if (! CHECK_FLAG (peer->sflags, PEER_STATUS_GROUP)) { - peer_unlock (peer); /* bgp peer list reference */ - listnode_delete (bgp->peer, peer); + struct listnode *pn; + + if ((pn = listnode_lookup (bgp->peer, peer))) + { + peer_unlock (peer); /* bgp peer list reference */ + list_delete_node (bgp->peer, pn); + } - if (peer_rsclient_active (peer)) + if (peer_rsclient_active (peer) + && (pn = listnode_lookup (bgp->rsclient, peer))) { peer_unlock (peer); /* rsclient list reference */ - listnode_delete (bgp->rsclient, peer); + list_delete_node (bgp->rsclient, pn); } } @@ -1219,8 +1259,6 @@ peer_delete (struct peer *peer) sockunion_free (peer->su_remote); peer->su_local = peer->su_remote = NULL; - bgp_sync_delete (peer); - /* Free filter related memory. */ for (afi = AFI_IP; afi < AFI_MAX; afi++) for (safi = SAFI_UNICAST; safi < SAFI_MAX; safi++) @@ -1692,7 +1730,7 @@ peer_group_bind (struct bgp *bgp, union peer->group = group; peer->af_group[afi][safi] = 1; - peer = peer_lock (peer); /* peer-group group->peer reference */ + peer = peer_lock (peer); /* group->peer list reference */ listnode_add (group->peer, peer); peer_group2peer_config_copy (group, peer, afi, safi); @@ -1733,9 +1771,11 @@ peer_group_bind (struct bgp *bgp, union { peer->group = group; - peer = peer_lock (peer); /* peer-group group->peer reference */ + peer = peer_lock (peer); /* group->peer list reference */ listnode_add (group->peer, peer); } + else + assert (group && peer->group == group); if (first_member) { @@ -1759,13 +1799,16 @@ peer_group_bind (struct bgp *bgp, union if (CHECK_FLAG(peer->af_flags[afi][safi], PEER_FLAG_RSERVER_CLIENT)) { + struct listnode *pn; + /* If it's not configured as RSERVER_CLIENT in any other address family, without being member of a peer_group, remove it from list bgp->rsclient.*/ - if (! peer_rsclient_active (peer)) + if (! peer_rsclient_active (peer) + && (pn = listnode_lookup (bgp->rsclient, peer))) { peer_unlock (peer); /* peer rsclient reference */ - listnode_delete (bgp->rsclient, peer); + list_delete_node (bgp->rsclient, pn); } bgp_table_finish (peer->rib[afi][safi]); @@ -1821,7 +1864,8 @@ peer_group_unbind (struct bgp *bgp, stru if (! peer_group_active (peer)) { - peer_unlock (peer); /* peer group list reference */ + assert (listnode_lookup (group->peer, peer)); + peer_unlock (peer); /* group->peer list reference */ listnode_delete (group->peer, peer); peer->group = NULL; if (group->conf->as) diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h index 9e2aa3e..1147978 100644 --- a/bgpd/bgpd.h +++ b/bgpd/bgpd.h @@ -386,7 +386,6 @@ #define PEER_STATUS_HAVE_ACCEPT (1 #define PEER_STATUS_GROUP (1 << 4) /* peer-group conf */ #define PEER_STATUS_NSF_MODE (1 << 5) /* NSF aware peer */ #define PEER_STATUS_NSF_WAIT (1 << 6) /* wait comeback peer */ -#define PEER_STATUS_CLEARING (1 << 7) /* peers table being cleared */ /* Peer status af flags (reset in bgp_stop) */ u_int16_t af_sflags[AFI_MAX][SAFI_MAX]; @@ -662,7 +661,9 @@ #define Active #define OpenSent 4 #define OpenConfirm 5 #define Established 6 -#define BGP_STATUS_MAX 7 +#define Clearing 7 +#define Deleted 8 +#define BGP_STATUS_MAX 9 /* BGP finite state machine events. */ #define BGP_Start 1 @@ -678,7 +679,9 @@ #define Receive_OPEN_message #define Receive_KEEPALIVE_message 11 #define Receive_UPDATE_message 12 #define Receive_NOTIFICATION_message 13 -#define BGP_EVENTS_MAX 14 +#define Clearing_Completed 14 +#define Prefix_Overflowed 15 +#define BGP_EVENTS_MAX 16 /* BGP timers default value. */ #define BGP_INIT_START_TIMER 5