bgpd: Implement group-overrides for peer timers

This commit implements BGP peer-group overrides for the timer flags,
which control the value of the hold, keepalive, advertisement-interval
and connect connect timers. It was kept separated on purpose as the
whole timer implementation is quite complex and merging this commit
together with with the other flag implementations did not seem right.

Basically three new peer flags were introduced, namely
*PEER_FLAG_ROUTEADV*, *PEER_FLAG_TIMER* and *PEER_FLAG_TIMER_CONNECT*.
The overrides work exactly the same way as they did before, but
introducing these flags made a few conditionals simpler as they no
longer had to compare internal data structures against eachother.

Last but not least, the test suite has been adjusted accordingly to test
the newly implemented flag overrides.

Signed-off-by: Pascal Mathis <mail@pascalmathis.com>
(cherry picked from commit b90a8e13eea49404d72dd0cdf94343d8a1831d69)

diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c
index a11a4f78f772732dc77b9a916539a57a7c3e961a..d0534c3eab7d8d8c2545d737738eccfe124e25cc 100644 (file)
--- a/bgpd/bgp_fsm.c
+++ b/bgpd/bgp_fsm.c
@@ -193,7 +193,6 @@ static struct peer *peer_xfer_conn(struct peer *from_peer)
        peer->as = from_peer->as;
        peer->v_holdtime = from_peer->v_holdtime;
        peer->v_keepalive = from_peer->v_keepalive;
-       peer->routeadv = from_peer->routeadv;
        peer->v_routeadv = from_peer->v_routeadv;
        peer->v_gr_restart = from_peer->v_gr_restart;
        peer->cap = from_peer->cap;
@@ -1143,7 +1142,7 @@ int bgp_stop(struct peer *peer)
        }
 
        /* Reset keepalive and holdtime */
-       if (PEER_OR_GROUP_TIMER_SET(peer)) {
+       if (CHECK_FLAG(peer->flags, PEER_FLAG_TIMER)) {
                peer->v_keepalive = peer->keepalive;
                peer->v_holdtime = peer->holdtime;
        } else {

diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c
index 9ea2f22cec15c9ea0285abc54b7e79f218576963..446dc5ac1254dece034006922c2d0afac5e6d719 100644 (file)
--- a/bgpd/bgp_packet.c
+++ b/bgpd/bgp_packet.c
@@ -503,7 +503,7 @@ void bgp_open_send(struct peer *peer)
        uint16_t send_holdtime;
        as_t local_as;
 
-       if (PEER_OR_GROUP_TIMER_SET(peer))
+       if (CHECK_FLAG(peer->flags, PEER_FLAG_TIMER))
                send_holdtime = peer->holdtime;
        else
                send_holdtime = peer->bgp->default_holdtime;
@@ -1237,7 +1237,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 (PEER_OR_GROUP_TIMER_SET(peer))
+       if (CHECK_FLAG(peer->flags, PEER_FLAG_TIMER))
                send_holdtime = peer->holdtime;
        else
                send_holdtime = peer->bgp->default_holdtime;
@@ -1247,7 +1247,7 @@ static int bgp_open_receive(struct peer *peer, bgp_size_t size)
        else
                peer->v_holdtime = send_holdtime;
 
-       if ((PEER_OR_GROUP_TIMER_SET(peer))
+       if ((CHECK_FLAG(peer->flags, PEER_FLAG_TIMER))
            && (peer->keepalive < peer->v_holdtime / 3))
                peer->v_keepalive = peer->keepalive;
        else

diff --git a/bgpd/bgp_snmp.c b/bgpd/bgp_snmp.c
index 241b23a62db758d0320aa046ed9ca2c5e537faed..0700e0ac242e63003431c82ee1d934ba1e611aee 100644 (file)
--- a/bgpd/bgp_snmp.c
+++ b/bgpd/bgp_snmp.c
@@ -487,17 +487,17 @@ static int write_bgpPeerTable(int action, uint8_t *var_val,
                        return SNMP_ERR_NOSUCHNAME;
                break;
        case BGPPEERCONNECTRETRYINTERVAL:
-               SET_FLAG(peer->config, PEER_CONFIG_CONNECT);
+               peer_flag_set(peer, PEER_FLAG_TIMER_CONNECT);
                peer->connect = intval;
                peer->v_connect = intval;
                break;
        case BGPPEERHOLDTIMECONFIGURED:
-               SET_FLAG(peer->config, PEER_CONFIG_TIMER);
+               peer_flag_set(peer, PEER_FLAG_TIMER);
                peer->holdtime = intval;
                peer->v_holdtime = intval;
                break;
        case BGPPEERKEEPALIVECONFIGURED:
-               SET_FLAG(peer->config, PEER_CONFIG_TIMER);
+               peer_flag_set(peer, PEER_FLAG_TIMER);
                peer->keepalive = intval;
                peer->v_keepalive = intval;
                break;
@@ -617,14 +617,14 @@ static uint8_t *bgpPeerTable(struct variable *v, oid name[], size_t *length,
                break;
        case BGPPEERHOLDTIMECONFIGURED:
                *write_method = write_bgpPeerTable;
-               if (PEER_OR_GROUP_TIMER_SET(peer))
+               if (CHECK_FLAG(peer->flags, PEER_FLAG_TIMER))
                        return SNMP_INTEGER(peer->holdtime);
                else
                        return SNMP_INTEGER(peer->v_holdtime);
                break;
        case BGPPEERKEEPALIVECONFIGURED:
                *write_method = write_bgpPeerTable;
-               if (PEER_OR_GROUP_TIMER_SET(peer))
+               if (CHECK_FLAG(peer->flags, PEER_FLAG_TIMER))
                        return SNMP_INTEGER(peer->keepalive);
                else
                        return SNMP_INTEGER(peer->v_keepalive);

diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c
index f588d0e7fa508e72e3ade1bc8fc6c90e60cfe551..6a3ff59bc615fe87a7ba8f3b04f1fdd323c9ceb9 100644 (file)
--- a/bgpd/bgp_vty.c
+++ b/bgpd/bgp_vty.c
@@ -9179,8 +9179,7 @@ static void bgp_show_peer(struct vty *vty, struct peer *p, uint8_t use_json,
                json_object_int_add(json_neigh,
                                    "bgpTimerKeepAliveIntervalMsecs",
                                    p->v_keepalive * 1000);
-
-               if (PEER_OR_GROUP_TIMER_SET(p)) {
+               if (CHECK_FLAG(p->flags, PEER_FLAG_TIMER)) {
                        json_object_int_add(json_neigh,
                                            "bgpTimerConfiguredHoldTimeMsecs",
                                            p->holdtime * 1000);
@@ -9244,7 +9243,7 @@ static void bgp_show_peer(struct vty *vty, struct peer *p, uint8_t use_json,
                vty_out(vty,
                        "  Hold time is %d, keepalive interval is %d seconds\n",
                        p->v_holdtime, p->v_keepalive);
-               if (PEER_OR_GROUP_TIMER_SET(p)) {
+               if (CHECK_FLAG(p->flags, PEER_FLAG_TIMER)) {
                        vty_out(vty, "  Configured hold time is %d",
                                p->holdtime);
                        vty_out(vty, ", keepalive interval is %d seconds\n",

diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index 54eb72d46f74a208470f50b4d46dc1f32fc6b62c..cdd1b40751aee7d5dcf85e64a475ed4c927d6065 100644 (file)
--- a/bgpd/bgpd.c
+++ b/bgpd/bgpd.c
@@ -992,7 +992,6 @@ static void peer_global_config_reset(struct peer *peer)
        peer->flags = 0;
        SET_FLAG(peer->flags, saved_flags);
 
-       peer->config = 0;
        peer->holdtime = 0;
        peer->keepalive = 0;
        peer->connect = 0;
@@ -1303,7 +1302,6 @@ void peer_xfer_config(struct peer *peer_dst, struct peer *peer_src)
        /* peer flags apply */
        peer_dst->flags = peer_src->flags;
        peer_dst->cap = peer_src->cap;
-       peer_dst->config = peer_src->config;
 
        peer_dst->local_as = peer_src->local_as;
        peer_dst->port = peer_src->port;
@@ -1563,10 +1561,9 @@ struct peer *peer_create(union sockunion *su, const char *conf_if,
        peer->local_id = bgp->router_id;
        peer->v_holdtime = bgp->default_holdtime;
        peer->v_keepalive = bgp->default_keepalive;
-       if (peer_sort(peer) == BGP_PEER_IBGP)
-               peer->v_routeadv = BGP_DEFAULT_IBGP_ROUTEADV;
-       else
-               peer->v_routeadv = BGP_DEFAULT_EBGP_ROUTEADV;
+       peer->v_routeadv = (peer_sort(peer) == BGP_PEER_IBGP)
+                                  ? BGP_DEFAULT_IBGP_ROUTEADV
+                                  : BGP_DEFAULT_EBGP_ROUTEADV;
 
        peer = peer_lock(peer); /* bgp peer list reference */
        peer->group = group;
@@ -1642,7 +1639,6 @@ int bgp_afi_safi_peer_exists(struct bgp *bgp, afi_t afi, safi_t safi)
 void peer_as_change(struct peer *peer, as_t as, int as_specified)
 {
        bgp_peer_sort_t type;
-       struct peer *conf;
 
        /* Stop peer. */
        if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
@@ -1665,22 +1661,12 @@ void peer_as_change(struct peer *peer, as_t as, int as_specified)
                peer->local_as = peer->bgp->as;
 
        /* Advertisement-interval reset */
-       conf = NULL;
-       if (peer->group)
-               conf = peer->group->conf;
-
-       if (conf && CHECK_FLAG(conf->config, PEER_CONFIG_ROUTEADV)) {
-               peer->v_routeadv = conf->routeadv;
-       }
-       /* Only go back to the default advertisement-interval if the user had
-        * not
-        * already configured it */
-       else if (!CHECK_FLAG(peer->config, PEER_CONFIG_ROUTEADV)) {
-               if (peer_sort(peer) == BGP_PEER_IBGP)
-                       peer->v_routeadv = BGP_DEFAULT_IBGP_ROUTEADV;
-               else
-                       peer->v_routeadv = BGP_DEFAULT_EBGP_ROUTEADV;
+       if (!CHECK_FLAG(peer->flags, PEER_FLAG_ROUTEADV)) {
+               peer->v_routeadv = (peer_sort(peer) == BGP_PEER_IBGP)
+                                          ? BGP_DEFAULT_IBGP_ROUTEADV
+                                          : BGP_DEFAULT_EBGP_ROUTEADV;
        }
+
        /* TTL reset */
        if (peer_sort(peer) == BGP_PEER_IBGP)
                peer->ttl = MAXTTL;
@@ -2406,12 +2392,6 @@ struct peer_group *peer_group_get(struct bgp *bgp, const char *name)
        group->conf->ttl = 1;
        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;
-       group->conf->connect = 0;
        SET_FLAG(group->conf->sflags, PEER_STATUS_GROUP);
        listnode_add_sort(bgp->group, group);
 
@@ -2443,25 +2423,30 @@ static void peer_group2peer_config_copy(struct peer_group *group,
        peer->flags |= conf->flags & ~peer->flags_override;
        peer->flags_invert |= conf->flags_invert;
 
-       /* peer config apply */
-       peer->config = conf->config;
-
        /* peer timers apply */
-       peer->holdtime = conf->holdtime;
-       peer->keepalive = conf->keepalive;
-       peer->connect = conf->connect;
-       if (CHECK_FLAG(conf->config, PEER_CONFIG_CONNECT))
-               peer->v_connect = conf->connect;
-       else
-               peer->v_connect = BGP_DEFAULT_CONNECT_RETRY;
+       if (!CHECK_FLAG(peer->flags_override, PEER_FLAG_TIMER)) {
+               PEER_ATTR_INHERIT(peer, group, holdtime);
+               PEER_ATTR_INHERIT(peer, group, keepalive);
+       }
 
-       /* advertisement-interval reset */
-       if (CHECK_FLAG(conf->config, PEER_CONFIG_ROUTEADV))
-               peer->v_routeadv = conf->routeadv;
-       else if (peer_sort(peer) == BGP_PEER_IBGP)
-               peer->v_routeadv = BGP_DEFAULT_IBGP_ROUTEADV;
-       else
-               peer->v_routeadv = BGP_DEFAULT_EBGP_ROUTEADV;
+       if (!CHECK_FLAG(peer->flags_override, PEER_FLAG_TIMER_CONNECT)) {
+               PEER_ATTR_INHERIT(peer, group, connect);
+               if (CHECK_FLAG(conf->flags, PEER_FLAG_TIMER_CONNECT))
+                       peer->v_connect = conf->connect;
+               else
+                       peer->v_connect = BGP_DEFAULT_CONNECT_RETRY;
+       }
+
+       /* advertisement-interval apply */
+       if (!CHECK_FLAG(peer->flags_override, PEER_FLAG_ROUTEADV)) {
+               PEER_ATTR_INHERIT(peer, group, routeadv);
+               if (CHECK_FLAG(conf->flags, PEER_FLAG_ROUTEADV))
+                       peer->v_routeadv = conf->routeadv;
+               else
+                       peer->v_routeadv = (peer_sort(peer) == BGP_PEER_IBGP)
+                                                  ? BGP_DEFAULT_IBGP_ROUTEADV
+                                                  : BGP_DEFAULT_EBGP_ROUTEADV;
+       }
 
        /* password apply */
        if (conf->password && !peer->password)
@@ -2750,14 +2735,13 @@ int peer_group_bind(struct bgp *bgp, union sockunion *su, struct peer *peer,
 
                if (first_member) {
                        /* Advertisement-interval reset */
-                       if (!CHECK_FLAG(group->conf->config,
-                                       PEER_CONFIG_ROUTEADV)) {
-                               if (peer_sort(group->conf) == BGP_PEER_IBGP)
-                                       group->conf->v_routeadv =
-                                               BGP_DEFAULT_IBGP_ROUTEADV;
-                               else
-                                       group->conf->v_routeadv =
-                                               BGP_DEFAULT_EBGP_ROUTEADV;
+                       if (!CHECK_FLAG(group->conf->flags,
+                                       PEER_FLAG_ROUTEADV)) {
+                               group->conf->v_routeadv =
+                                       (peer_sort(group->conf)
+                                        == BGP_PEER_IBGP)
+                                               ? BGP_DEFAULT_IBGP_ROUTEADV
+                                               : BGP_DEFAULT_EBGP_ROUTEADV;
                        }
 
                        /* ebgp-multihop reset */
@@ -3819,6 +3803,9 @@ static const struct peer_flag_action peer_flag_action_list[] = {
        {PEER_FLAG_DISABLE_CONNECTED_CHECK, 0, peer_change_reset},
        {PEER_FLAG_CAPABILITY_ENHE, 0, peer_change_reset},
        {PEER_FLAG_ENFORCE_FIRST_AS, 0, peer_change_reset_in},
+       {PEER_FLAG_ROUTEADV, 0, peer_change_none},
+       {PEER_FLAG_TIMER, 0, peer_change_none},
+       {PEER_FLAG_TIMER_CONNECT, 0, peer_change_none},
        {0, 0, 0}};
 
 static const struct peer_flag_action peer_af_flag_action_list[] = {
@@ -4850,44 +4837,40 @@ int peer_weight_unset(struct peer *peer, afi_t afi, safi_t safi)
 
 int peer_timers_set(struct peer *peer, uint32_t keepalive, uint32_t holdtime)
 {
-       struct peer_group *group;
+       struct peer *member;
        struct listnode *node, *nnode;
 
-       /* keepalive value check.  */
        if (keepalive > 65535)
                return BGP_ERR_INVALID_VALUE;
 
-       /* Holdtime value check.  */
        if (holdtime > 65535)
                return BGP_ERR_INVALID_VALUE;
 
-       /* Holdtime value must be either 0 or greater than 3.  */
        if (holdtime < 3 && holdtime != 0)
                return BGP_ERR_INVALID_VALUE;
 
-       /* Set value to the configuration. */
+       /* Set flag and configuration on peer. */
+       peer_flag_set(peer, PEER_FLAG_TIMER);
        peer->holdtime = holdtime;
        peer->keepalive = (keepalive < holdtime / 3 ? keepalive : holdtime / 3);
 
-       /* First work on real peers with timers */
-       if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
-               SET_FLAG(peer->config, PEER_CONFIG_TIMER);
-               UNSET_FLAG(peer->config, PEER_GROUP_CONFIG_TIMER);
-       } else {
-               /* Now work on the peer-group timers */
-               SET_FLAG(peer->config, PEER_GROUP_CONFIG_TIMER);
+       /* Skip peer-group mechanics for regular peers. */
+       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)) {
-                       /* Skip peers that have their own timers */
-                       if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER))
-                               continue;
+       /*
+        * Set flag and configuration on all peer-group members, unless they are
+        * explicitely overriding peer-group configuration.
+        */
+       for (ALL_LIST_ELEMENTS(peer->group->peer, node, nnode, member)) {
+               /* Skip peers with overridden configuration. */
+               if (CHECK_FLAG(member->flags_override, PEER_FLAG_TIMER))
+                       continue;
 
-                       SET_FLAG(peer->config, PEER_GROUP_CONFIG_TIMER);
-                       peer->holdtime = group->conf->holdtime;
-                       peer->keepalive = group->conf->keepalive;
-               }
+               /* Set flag and configuration on peer-group member. */
+               SET_FLAG(member->flags, PEER_FLAG_TIMER);
+               PEER_ATTR_INHERIT(peer, peer->group, holdtime);
+               PEER_ATTR_INHERIT(peer, peer->group, keepalive);
        }
 
        return 0;
@@ -4895,35 +4878,38 @@ int peer_timers_set(struct peer *peer, uint32_t keepalive, uint32_t holdtime)
 
 int peer_timers_unset(struct peer *peer)
 {
-       struct peer_group *group;
+       struct peer *member;
        struct listnode *node, *nnode;
 
-       /* First work on real peers vs the peer-group */
-       if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
-               UNSET_FLAG(peer->config, PEER_CONFIG_TIMER);
-               peer->keepalive = 0;
+       /* Inherit configuration from peer-group if peer is member. */
+       if (peer_group_active(peer)) {
+               peer_flag_inherit(peer, PEER_FLAG_TIMER);
+               PEER_ATTR_INHERIT(peer, peer->group, holdtime);
+               PEER_ATTR_INHERIT(peer, peer->group, keepalive);
+       } else {
+               /* Otherwise remove flag and configuration from peer. */
+               peer_flag_unset(peer, PEER_FLAG_TIMER);
                peer->holdtime = 0;
+               peer->keepalive = 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;
-                       }
-               }
+       /* Skip peer-group mechanics for regular peers. */
+       if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP))
+               return 0;
+
+       /*
+        * Remove flag and configuration from all peer-group members, unless
+        * they are explicitely overriding peer-group configuration.
+        */
+       for (ALL_LIST_ELEMENTS(peer->group->peer, node, nnode, member)) {
+               /* Skip peers with overridden configuration. */
+               if (CHECK_FLAG(member->flags_override, PEER_FLAG_TIMER))
+                       continue;
 
-               UNSET_FLAG(group->conf->config, PEER_GROUP_CONFIG_TIMER);
-               group->conf->holdtime = 0;
-               group->conf->keepalive = 0;
+               /* Remove flag and configuration on peer-group member. */
+               UNSET_FLAG(member->flags, PEER_FLAG_TIMER);
+               member->holdtime = 0;
+               member->keepalive = 0;
        }
 
        return 0;
@@ -4931,85 +4917,124 @@ int peer_timers_unset(struct peer *peer)
 
 int peer_timers_connect_set(struct peer *peer, uint32_t connect)
 {
-       struct peer_group *group;
+       struct peer *member;
        struct listnode *node, *nnode;
 
        if (connect > 65535)
                return BGP_ERR_INVALID_VALUE;
 
-       /* Set value to the configuration. */
-       SET_FLAG(peer->config, PEER_CONFIG_CONNECT);
+       /* Set flag and configuration on peer. */
+       peer_flag_set(peer, PEER_FLAG_TIMER_CONNECT);
        peer->connect = connect;
-
-       /* Set value to timer setting. */
        peer->v_connect = connect;
 
+       /* Skip peer-group mechanics for regular peers. */
        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)) {
-               SET_FLAG(peer->config, PEER_CONFIG_CONNECT);
-               peer->connect = connect;
-               peer->v_connect = connect;
+       /*
+        * Set flag and configuration on all peer-group members, unless they are
+        * explicitely overriding peer-group configuration.
+        */
+       for (ALL_LIST_ELEMENTS(peer->group->peer, node, nnode, member)) {
+               /* Skip peers with overridden configuration. */
+               if (CHECK_FLAG(member->flags_override, PEER_FLAG_TIMER_CONNECT))
+                       continue;
+
+               /* Set flag and configuration on peer-group member. */
+               SET_FLAG(member->flags, PEER_FLAG_TIMER_CONNECT);
+               member->connect = connect;
+               member->v_connect = connect;
        }
+
        return 0;
 }
 
 int peer_timers_connect_unset(struct peer *peer)
 {
-       struct peer_group *group;
+       struct peer *member;
        struct listnode *node, *nnode;
 
-       /* Clear configuration. */
-       UNSET_FLAG(peer->config, PEER_CONFIG_CONNECT);
-       peer->connect = 0;
+       /* Inherit configuration from peer-group if peer is member. */
+       if (peer_group_active(peer)) {
+               peer_flag_inherit(peer, PEER_FLAG_TIMER_CONNECT);
+               PEER_ATTR_INHERIT(peer, peer->group, connect);
+       } else {
+               /* Otherwise remove flag and configuration from peer. */
+               peer_flag_unset(peer, PEER_FLAG_TIMER_CONNECT);
+               peer->connect = 0;
+       }
 
-       /* Set timer setting to default value. */
-       peer->v_connect = BGP_DEFAULT_CONNECT_RETRY;
+       /* Set timer with fallback to default value. */
+       if (peer->connect)
+               peer->v_connect = peer->connect;
+       else
+               peer->v_connect = BGP_DEFAULT_CONNECT_RETRY;
 
+       /* Skip peer-group mechanics for regular peers. */
        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_CONNECT);
-               peer->connect = 0;
-               peer->v_connect = BGP_DEFAULT_CONNECT_RETRY;
+       /*
+        * Remove flag and configuration from all peer-group members, unless
+        * they are explicitely overriding peer-group configuration.
+        */
+       for (ALL_LIST_ELEMENTS(peer->group->peer, node, nnode, member)) {
+               /* Skip peers with overridden configuration. */
+               if (CHECK_FLAG(member->flags_override, PEER_FLAG_TIMER_CONNECT))
+                       continue;
+
+               /* Remove flag and configuration on peer-group member. */
+               UNSET_FLAG(member->flags, PEER_FLAG_TIMER_CONNECT);
+               member->connect = 0;
+               member->v_connect = BGP_DEFAULT_CONNECT_RETRY;
        }
+
        return 0;
 }
 
 int peer_advertise_interval_set(struct peer *peer, uint32_t routeadv)
 {
-       struct peer_group *group;
+       struct peer *member;
        struct listnode *node, *nnode;
 
        if (routeadv > 600)
                return BGP_ERR_INVALID_VALUE;
 
-       SET_FLAG(peer->config, PEER_CONFIG_ROUTEADV);
+       /* Set flag and configuration on peer. */
+       peer_flag_set(peer, PEER_FLAG_ROUTEADV);
        peer->routeadv = routeadv;
        peer->v_routeadv = routeadv;
 
+       /* Check if handling a regular peer. */
        if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
+               /* Update peer route announcements. */
                update_group_adjust_peer_afs(peer);
                if (peer->status == Established)
                        bgp_announce_route_all(peer);
+
+               /* Skip peer-group mechanics for regular peers. */
                return 0;
        }
 
-       /* peer-group member updates. */
-       group = peer->group;
-       for (ALL_LIST_ELEMENTS(group->peer, node, nnode, peer)) {
-               SET_FLAG(peer->config, PEER_CONFIG_ROUTEADV);
-               peer->routeadv = routeadv;
-               peer->v_routeadv = routeadv;
-               update_group_adjust_peer_afs(peer);
-               if (peer->status == Established)
-                       bgp_announce_route_all(peer);
+       /*
+        * Set flag and configuration on all peer-group members, unless they are
+        * explicitely overriding peer-group configuration.
+        */
+       for (ALL_LIST_ELEMENTS(peer->group->peer, node, nnode, member)) {
+               /* Skip peers with overridden configuration. */
+               if (CHECK_FLAG(member->flags_override, PEER_FLAG_ROUTEADV))
+                       continue;
+
+               /* Set flag and configuration on peer-group member. */
+               SET_FLAG(member->flags, PEER_FLAG_ROUTEADV);
+               member->routeadv = routeadv;
+               member->v_routeadv = routeadv;
+
+               /* Update peer route announcements. */
+               update_group_adjust_peer_afs(member);
+               if (member->status == Established)
+                       bgp_announce_route_all(member);
        }
 
        return 0;
@@ -5017,38 +5042,58 @@ int peer_advertise_interval_set(struct peer *peer, uint32_t routeadv)
 
 int peer_advertise_interval_unset(struct peer *peer)
 {
-       struct peer_group *group;
+       struct peer *member;
        struct listnode *node, *nnode;
 
-       UNSET_FLAG(peer->config, PEER_CONFIG_ROUTEADV);
-       peer->routeadv = 0;
+       /* Inherit configuration from peer-group if peer is member. */
+       if (peer_group_active(peer)) {
+               peer_flag_inherit(peer, PEER_FLAG_ROUTEADV);
+               PEER_ATTR_INHERIT(peer, peer->group, routeadv);
+       } else {
+               /* Otherwise remove flag and configuration from peer. */
+               peer_flag_unset(peer, PEER_FLAG_ROUTEADV);
+               peer->routeadv = 0;
+       }
 
-       if (peer->sort == BGP_PEER_IBGP)
-               peer->v_routeadv = BGP_DEFAULT_IBGP_ROUTEADV;
+       /* Set timer with fallback to default value. */
+       if (peer->routeadv)
+               peer->v_routeadv = peer->routeadv;
        else
-               peer->v_routeadv = BGP_DEFAULT_EBGP_ROUTEADV;
+               peer->v_routeadv = (peer->sort == BGP_PEER_IBGP)
+                                          ? BGP_DEFAULT_IBGP_ROUTEADV
+                                          : BGP_DEFAULT_EBGP_ROUTEADV;
 
+       /* Check if handling a regular peer. */
        if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
+               /* Update peer route announcements. */
                update_group_adjust_peer_afs(peer);
                if (peer->status == Established)
                        bgp_announce_route_all(peer);
+
+               /* Skip peer-group mechanics for regular peers. */
                return 0;
        }
 
-       /* peer-group member updates. */
-       group = peer->group;
-       for (ALL_LIST_ELEMENTS(group->peer, node, nnode, peer)) {
-               UNSET_FLAG(peer->config, PEER_CONFIG_ROUTEADV);
-               peer->routeadv = 0;
+       /*
+        * Remove flag and configuration from all peer-group members, unless
+        * they are explicitely overriding peer-group configuration.
+        */
+       for (ALL_LIST_ELEMENTS(peer->group->peer, node, nnode, member)) {
+               /* Skip peers with overridden configuration. */
+               if (CHECK_FLAG(member->flags_override, PEER_FLAG_ROUTEADV))
+                       continue;
 
-               if (peer->sort == BGP_PEER_IBGP)
-                       peer->v_routeadv = BGP_DEFAULT_IBGP_ROUTEADV;
-               else
-                       peer->v_routeadv = BGP_DEFAULT_EBGP_ROUTEADV;
+               /* Remove flag and configuration on peer-group member. */
+               UNSET_FLAG(member->flags, PEER_FLAG_ROUTEADV);
+               member->routeadv = 0;
+               member->v_routeadv = (member->sort == BGP_PEER_IBGP)
+                                            ? BGP_DEFAULT_IBGP_ROUTEADV
+                                            : BGP_DEFAULT_EBGP_ROUTEADV;
 
-               update_group_adjust_peer_afs(peer);
-               if (peer->status == Established)
-                       bgp_announce_route_all(peer);
+               /* Update peer route announcements. */
+               update_group_adjust_peer_afs(member);
+               if (member->status == Established)
+                       bgp_announce_route_all(member);
        }
 
        return 0;
@@ -6936,37 +6981,19 @@ static void bgp_config_write_peer_global(struct vty *vty, struct bgp *bgp,
        }
 
        /* advertisement-interval */
-       if (CHECK_FLAG(peer->config, PEER_CONFIG_ROUTEADV)
-           && ((!peer_group_active(peer)
-                && peer->v_routeadv != BGP_DEFAULT_EBGP_ROUTEADV)
-               || (peer_group_active(peer)
-                   && peer->v_routeadv != g_peer->v_routeadv))) {
+       if (peergroup_flag_check(peer, PEER_FLAG_ROUTEADV))
                vty_out(vty, " neighbor %s advertisement-interval %u\n", addr,
-                       peer->v_routeadv);
-       }
+                       peer->routeadv);
 
        /* timers */
-       if ((PEER_OR_GROUP_TIMER_SET(peer))
-           && ((!peer_group_active(peer)
-                && (peer->keepalive != BGP_DEFAULT_KEEPALIVE
-                    || peer->holdtime != BGP_DEFAULT_HOLDTIME))
-               || (peer_group_active(peer)
-                   && (peer->keepalive != g_peer->keepalive
-                       || peer->holdtime != g_peer->holdtime)))) {
+       if (peergroup_flag_check(peer, PEER_FLAG_TIMER))
                vty_out(vty, " neighbor %s timers %u %u\n", addr,
                        peer->keepalive, peer->holdtime);
-       }
 
-       if (CHECK_FLAG(peer->config, PEER_CONFIG_CONNECT)
-           && ((!peer_group_active(peer)
-                && peer->connect != BGP_DEFAULT_CONNECT_RETRY)
-               || (peer_group_active(peer)
-                   && peer->connect != g_peer->connect)))
-
-       {
+       /* timers connect */
+       if (peergroup_flag_check(peer, PEER_FLAG_TIMER_CONNECT))
                vty_out(vty, " neighbor %s timers connect %u\n", addr,
                        peer->connect);
-       }
 
        /* capability dynamic */
        if (peergroup_flag_check(peer, PEER_FLAG_DYNAMIC_CAPABILITY))

diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h
index 06c9b0b72f36fc7d148e63b4e9534fac62e28665..9a4946f960eee268050c52690b7a3fdd7bedee38 100644 (file)
--- a/bgpd/bgpd.h
+++ b/bgpd/bgpd.h
@@ -888,6 +888,9 @@ struct peer {
 #define PEER_FLAG_IFPEER_V6ONLY             (1 << 14) /* if-based peer is v6 only */
 #define PEER_FLAG_IS_RFAPI_HD               (1 << 15) /* attached to rfapi HD */
 #define PEER_FLAG_ENFORCE_FIRST_AS          (1 << 16) /* enforce-first-as */
+#define PEER_FLAG_ROUTEADV                  (1 << 17) /* route advertise */
+#define PEER_FLAG_TIMER                     (1 << 18) /* keepalive & holdtime */
+#define PEER_FLAG_TIMER_CONNECT             (1 << 19) /* connect timer */
 
        /* outgoing message sent in CEASE_ADMIN_SHUTDOWN notify */
        char *tx_shutdown_message;
@@ -961,17 +964,7 @@ struct peer {
 #define PEER_STATUS_EOR_SEND          (1 << 4) /* end-of-rib send to peer */
 #define PEER_STATUS_EOR_RECEIVED      (1 << 5) /* end-of-rib received from peer */
 
-       /* Default attribute value for the peer. */
-       uint32_t config;
-#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))
-
+       /* Configured timer values. */
        _Atomic uint32_t holdtime;
        _Atomic uint32_t keepalive;
        _Atomic uint32_t connect;

diff --git a/tests/bgpd/test_peer_attr.c b/tests/bgpd/test_peer_attr.c
index b61e13c4b4a49ed9f76bb2de68258fc70530772e..5e09bf61a91d75df1e16efcd51f73e8fd631e902 100644 (file)
--- a/tests/bgpd/test_peer_attr.c
+++ b/tests/bgpd/test_peer_attr.c
@@ -183,17 +183,6 @@ static char *str_printf(const char *fmt, ...)
        return buf;
 }
 
-static void _test_advertisement_interval(struct test *test, struct peer *peer,
-                                        struct peer *group, bool peer_set,
-                                        bool group_set)
-{
-       uint32_t def = BGP_DEFAULT_EBGP_ROUTEADV;
-
-       TEST_ASSERT_EQ(test, peer->v_routeadv,
-                      (peer_set || group_set) ? 10 : def);
-       TEST_ASSERT_EQ(test, group->v_routeadv, group_set ? 20 : def);
-}
-
 /* clang-format off */
 static struct test_peer_attr test_peer_attrs[] = {
        /* Peer Attributes */
@@ -201,8 +190,8 @@ static struct test_peer_attr test_peer_attrs[] = {
                .cmd = "advertisement-interval",
                .peer_cmd = "advertisement-interval 10",
                .group_cmd = "advertisement-interval 20",
-               .type = PEER_AT_GLOBAL_CUSTOM,
-               .custom_handler = &_test_advertisement_interval,
+               .u.flag = PEER_FLAG_ROUTEADV,
+               .type = PEER_AT_GLOBAL_FLAG,
        },
        {
                .cmd = "capability dynamic",
@@ -256,6 +245,20 @@ static struct test_peer_attr test_peer_attrs[] = {
                .u.flag = PEER_FLAG_STRICT_CAP_MATCH,
                .type = PEER_AT_GLOBAL_FLAG,
        },
+       {
+               .cmd = "timers",
+               .peer_cmd = "timers 10 30",
+               .group_cmd = "timers 20 60",
+               .u.flag = PEER_FLAG_TIMER,
+               .type = PEER_AT_GLOBAL_FLAG,
+       },
+       {
+               .cmd = "timers connect",
+               .peer_cmd = "timers connect 10",
+               .group_cmd = "timers connect 20",
+               .u.flag = PEER_FLAG_TIMER_CONNECT,
+               .type = PEER_AT_GLOBAL_FLAG,
+       },
 
        /* Address Family Attributes */
        {
@@ -541,6 +544,11 @@ static const char *str_from_attr_type(enum test_peer_attr_type at)
        }
 }
 
+static bool is_attr_type_global(enum test_peer_attr_type at)
+{
+       return at == PEER_AT_GLOBAL_FLAG || at == PEER_AT_GLOBAL_CUSTOM;
+}
+
 static void test_log(struct test *test, const char *fmt, ...)
 {
        va_list ap;
@@ -944,7 +952,6 @@ static void test_process(struct test *test, struct test_peer_attr *pa,
 static void test_peer_attr(struct test *test, struct test_peer_attr *pa)
 {
        int tc = 1;
-       bool is_address_family;
        const char *type;
        const char *ecp = pa->o.invert_peer ? "no " : "";
        const char *dcp = pa->o.invert_peer ? "" : "no ";
@@ -964,12 +971,8 @@ static void test_peer_attr(struct test *test, struct test_peer_attr *pa)
                return;
        }
 
-       is_address_family =
-               (pa->type == PEER_AT_AF_FLAG || pa->type == PEER_AT_AF_FILTER
-                || pa->type == PEER_AT_AF_CUSTOM);
-
        /* Test Preparation: Switch and activate address-family. */
-       if (is_address_family) {
+       if (!is_attr_type_global(pa->type)) {
                test_log(test, "prepare: switch address-family to [%s]",
                         afi_safi_print(pa->afi, pa->safi));
                test_execute(test, "address-family %s %s",
@@ -1026,7 +1029,7 @@ static void test_peer_attr(struct test *test, struct test_peer_attr *pa)
        g = test->group;
 
        /* Test Preparation: Switch and activate address-family. */
-       if (is_address_family) {
+       if (!is_attr_type_global(pa->type)) {
                test_log(test, "prepare: switch address-family to [%s]",
                         afi_safi_print(pa->afi, pa->safi));
                test_execute(test, "address-family %s %s",
@@ -1193,7 +1196,7 @@ int main(void)
                pa = &test_peer_attrs[i++];
 
                /* Just copy the peer attribute structure for global flags. */
-               if (pa->type == PEER_AT_GLOBAL_FLAG) {
+               if (is_attr_type_global(pa->type)) {
                        pac = XMALLOC(MTYPE_TMP, sizeof(struct test_peer_attr));
                        memcpy(pac, pa, sizeof(struct test_peer_attr));
                        listnode_add(pa_list, pac);