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>

diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c
index 3f5ff12cbc0306c19f86d983f8cda55d55401a24..2fd63531db757d42f7c76906583e251e78758f0d 100644 (file)
--- a/bgpd/bgp_fsm.c
+++ b/bgpd/bgp_fsm.c
@@ -194,7 +194,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;
@@ -1144,7 +1143,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 9857c5b7c859578a571923219c4fd0387223bcb4..57e900d97e6fbf616625a8b7ea455e3b5ae22866 100644 (file)
--- a/bgpd/bgp_vty.c
+++ b/bgpd/bgp_vty.c
@@ -9191,8 +9191,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);
@@ -9256,7 +9255,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 278e5b78686585e300c5ed117c55f2a657bfe449..4b150ad4e099e31e429e716c45f182f732a555f1 100644 (file)
--- a/bgpd/bgpd.c
+++ b/bgpd/bgpd.c
@@ -993,7 +993,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;
@@ -1309,7 +1308,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;
@@ -1576,10 +1574,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;
@@ -1655,7 +1652,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)) {
@@ -1678,22 +1674,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;
@@ -2419,12 +2405,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);
 
@@ -2456,25 +2436,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)
@@ -2763,14 +2748,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 */
@@ -3854,6 +3838,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[] = {
@@ -4914,44 +4901,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;
@@ -4959,35 +4942,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;
@@ -4995,85 +4981,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;
@@ -5081,38 +5106,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;
@@ -7000,37 +7045,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 fdc363d1f48a2d6cd7b3680d09e7fd6fe9d50249..3ce309db6fdb45151efed89a99c4f15d8239bc1f 100644 (file)
--- a/bgpd/bgpd.h
+++ b/bgpd/bgpd.h
@@ -895,6 +895,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;
@@ -968,17 +971,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);