Merge pull request #1370 from dslicenc/cm18408-bgp-timers

bgpd: fix various problems with hold/keepalive timers

bgpd/bgp_fsm.c

@@ -1096,7 +1096,7 @@ int bgp_stop(struct peer *peer)
 		}
 
 	/* Reset keepalive and holdtime */
-	if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER)) {
+	if (PEER_OR_GROUP_TIMER_SET(peer)) {
 		peer->v_keepalive = peer->keepalive;
 		peer->v_holdtime = peer->holdtime;
 	} else {

bgpd/bgp_packet.c

@@ -529,7 +529,7 @@ void bgp_open_send(struct peer *peer)
 	u_int16_t send_holdtime;
 	as_t local_as;
 
-	if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER))
+	if (PEER_OR_GROUP_TIMER_SET(peer))
 		send_holdtime = peer->holdtime;
 	else
 		send_holdtime = peer->bgp->default_holdtime;
@@ -1087,7 +1087,7 @@ static int bgp_open_receive(struct peer *peer, bgp_size_t size)
 	   implementation MAY adjust the rate at which it sends KEEPALIVE
 	   messages as a function of the Hold Time interval. */
 
-	if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER))
+	if (PEER_OR_GROUP_TIMER_SET(peer))
 		send_holdtime = peer->holdtime;
 	else
 		send_holdtime = peer->bgp->default_holdtime;
@@ -1097,7 +1097,8 @@ static int bgp_open_receive(struct peer *peer, bgp_size_t size)
 	else
 		peer->v_holdtime = send_holdtime;
 
-	if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER))
+	if ((PEER_OR_GROUP_TIMER_SET(peer))
+	    && (peer->keepalive < peer->v_holdtime / 3))
 		peer->v_keepalive = peer->keepalive;
 	else
 		peer->v_keepalive = peer->v_holdtime / 3;

bgpd/bgp_snmp.c

@@ -615,14 +615,14 @@ static u_char *bgpPeerTable(struct variable *v, oid name[], size_t *length,
 		break;
 	case BGPPEERHOLDTIMECONFIGURED:
 		*write_method = write_bgpPeerTable;
-		if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER))
+		if (PEER_OR_GROUP_TIMER_SET(peer))
 			return SNMP_INTEGER(peer->holdtime);
 		else
 			return SNMP_INTEGER(peer->v_holdtime);
 		break;
 	case BGPPEERKEEPALIVECONFIGURED:
 		*write_method = write_bgpPeerTable;
-		if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER))
+		if (PEER_OR_GROUP_TIMER_SET(peer))
 			return SNMP_INTEGER(peer->keepalive);
 		else
 			return SNMP_INTEGER(peer->v_keepalive);

bgpd/bgp_vty.c

@@ -8272,7 +8272,7 @@ static void bgp_show_peer(struct vty *vty, struct peer *p, u_char use_json,
 				    "bgpTimerKeepAliveIntervalMsecs",
 				    p->v_keepalive * 1000);
 
-		if (CHECK_FLAG(p->config, PEER_CONFIG_TIMER)) {
+		if (PEER_OR_GROUP_TIMER_SET(p)) {
 			json_object_int_add(json_neigh,
 					    "bgpTimerConfiguredHoldTimeMsecs",
 					    p->holdtime * 1000);
@@ -8280,6 +8280,16 @@ static void bgp_show_peer(struct vty *vty, struct peer *p, u_char use_json,
 				json_neigh,
 				"bgpTimerConfiguredKeepAliveIntervalMsecs",
 				p->keepalive * 1000);
+		} else if ((bgp->default_holdtime != BGP_DEFAULT_HOLDTIME)
+			   || (bgp->default_keepalive !=
+			       BGP_DEFAULT_KEEPALIVE)) {
+			json_object_int_add(json_neigh,
+					    "bgpTimerConfiguredHoldTimeMsecs",
+					    bgp->default_holdtime);
+			json_object_int_add(
+				json_neigh,
+				"bgpTimerConfiguredKeepAliveIntervalMsecs",
+				bgp->default_keepalive);
 		}
 	} else {
 		/* Administrative shutdown. */
@@ -8326,11 +8336,18 @@ static void bgp_show_peer(struct vty *vty, struct peer *p, u_char use_json,
 		vty_out(vty,
 			"  Hold time is %d, keepalive interval is %d seconds\n",
 			p->v_holdtime, p->v_keepalive);
-		if (CHECK_FLAG(p->config, PEER_CONFIG_TIMER)) {
+		if (PEER_OR_GROUP_TIMER_SET(p)) {
 			vty_out(vty, "  Configured hold time is %d",
 				p->holdtime);
 			vty_out(vty, ", keepalive interval is %d seconds\n",
 				p->keepalive);
+		} else if ((bgp->default_holdtime != BGP_DEFAULT_HOLDTIME)
+			   || (bgp->default_keepalive !=
+			       BGP_DEFAULT_KEEPALIVE)) {
+			vty_out(vty, "  Configured hold time is %d",
+				bgp->default_holdtime);
+			vty_out(vty, ", keepalive interval is %d seconds\n",
+				bgp->default_keepalive);
 		}
 	}
 	/* Capability. */

bgpd/bgpd.c

@@ -2301,6 +2301,7 @@ struct peer_group *peer_group_get(struct bgp *bgp, const char *name)
 	group->conf->gtsm_hops = 0;
 	group->conf->v_routeadv = BGP_DEFAULT_EBGP_ROUTEADV;
 	UNSET_FLAG(group->conf->config, PEER_CONFIG_TIMER);
+	UNSET_FLAG(group->conf->config, PEER_GROUP_CONFIG_TIMER);
 	UNSET_FLAG(group->conf->config, PEER_CONFIG_CONNECT);
 	group->conf->keepalive = 0;
 	group->conf->holdtime = 0;
@@ -4582,20 +4583,30 @@ int peer_timers_set(struct peer *peer, u_int32_t keepalive, u_int32_t holdtime)
 		return BGP_ERR_INVALID_VALUE;
 
 	/* Set value to the configuration. */
-	SET_FLAG(peer->config, PEER_CONFIG_TIMER);
 	peer->holdtime = holdtime;
 	peer->keepalive = (keepalive < holdtime / 3 ? keepalive : holdtime / 3);
 
-	if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP))
+	/* First work on real peers with timers */
-		return 0;
+	if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
-
-	/* peer-group member updates. */
-	group = peer->group;
-	for (ALL_LIST_ELEMENTS(group->peer, node, nnode, peer)) {
 		SET_FLAG(peer->config, PEER_CONFIG_TIMER);
-		peer->holdtime = group->conf->holdtime;
+		UNSET_FLAG(peer->config, PEER_GROUP_CONFIG_TIMER);
-		peer->keepalive = group->conf->keepalive;
+	} else {
+		/* Now work on the peer-group timers */
+		SET_FLAG(peer->config, PEER_GROUP_CONFIG_TIMER);
+
+		/* peer-group member updates. */
+		group = peer->group;
+		for (ALL_LIST_ELEMENTS(group->peer, node, nnode, peer)) {
+			/* Skip peers that have their own timers */
+			if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER))
+				continue;
+
+			SET_FLAG(peer->config, PEER_GROUP_CONFIG_TIMER);
+			peer->holdtime = group->conf->holdtime;
+			peer->keepalive = group->conf->keepalive;
+		}
 	}
+
 	return 0;
 }
 
@@ -4604,20 +4615,32 @@ int peer_timers_unset(struct peer *peer)
 	struct peer_group *group;
 	struct listnode *node, *nnode;
 
-	/* Clear configuration. */
+	/* First work on real peers vs the peer-group */
-	UNSET_FLAG(peer->config, PEER_CONFIG_TIMER);
+	if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
-	peer->keepalive = 0;
-	peer->holdtime = 0;
-
-	if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP))
-		return 0;
-
-	/* peer-group member updates. */
-	group = peer->group;
-	for (ALL_LIST_ELEMENTS(group->peer, node, nnode, peer)) {
 		UNSET_FLAG(peer->config, PEER_CONFIG_TIMER);
-		peer->holdtime = 0;
 		peer->keepalive = 0;
+		peer->holdtime = 0;
+
+		if (peer->group && peer->group->conf->holdtime) {
+			SET_FLAG(peer->config, PEER_GROUP_CONFIG_TIMER);
+			peer->keepalive = peer->group->conf->keepalive;
+			peer->holdtime = peer->group->conf->holdtime;
+		}
+	} else {
+		/* peer-group member updates. */
+		group = peer->group;
+		for (ALL_LIST_ELEMENTS(group->peer, node, nnode, peer)) {
+			if (!CHECK_FLAG(peer->config, PEER_CONFIG_TIMER)) {
+				UNSET_FLAG(peer->config,
+					   PEER_GROUP_CONFIG_TIMER);
+				peer->holdtime = 0;
+				peer->keepalive = 0;
+			}
+		}
+
+		UNSET_FLAG(group->conf->config, PEER_GROUP_CONFIG_TIMER);
+		group->conf->holdtime = 0;
+		group->conf->keepalive = 0;
 	}
 
 	return 0;
@@ -6535,7 +6558,7 @@ static void bgp_config_write_peer_global(struct vty *vty, struct bgp *bgp,
 	}
 
 	/* timers */
-	if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER)
+	if ((PEER_OR_GROUP_TIMER_SET(peer))
 	    && ((!peer_group_active(peer)
 		 && (peer->keepalive != BGP_DEFAULT_KEEPALIVE
 		     || peer->holdtime != BGP_DEFAULT_HOLDTIME))

bgpd/bgpd.h

@@ -771,6 +771,11 @@ struct peer {
 #define PEER_CONFIG_TIMER             (1 << 0) /* keepalive & holdtime */
 #define PEER_CONFIG_CONNECT           (1 << 1) /* connect */
 #define PEER_CONFIG_ROUTEADV          (1 << 2) /* route advertise */
+#define PEER_GROUP_CONFIG_TIMER       (1 << 3) /* timers from peer-group */
+
+#define PEER_OR_GROUP_TIMER_SET(peer)                                        \
+	(CHECK_FLAG(peer->config, PEER_CONFIG_TIMER)			     \
+	 || CHECK_FLAG(peer->config, PEER_GROUP_CONFIG_TIMER))
 
 	u_int32_t holdtime;
 	u_int32_t keepalive;