bgpd: refactored bmp_route_update & cleanup TODOs

TODOs that are done/un-necessary now deleted
refactored bmp_route_update to use a modified bmp_process_one function call instead of duplicating similar code

Signed-off-by: Maxence Younsi <mx.yns@outlook.fr>

diff --git a/bgpd/bgp_bmp.c b/bgpd/bgp_bmp.c
index 8ac17e22e88620e2a98597488971c0defb3a1b84..820720df55f8f450dc079b3675ff35f4e9d97520 100644 (file)
--- a/bgpd/bgp_bmp.c
+++ b/bgpd/bgp_bmp.c
@@ -1102,7 +1102,6 @@ afibreak:
                        prefix_copy(&bmp->syncpos, bgp_dest_get_prefix(bn));
                }
 
-               /* TODO BMP add BMP_MON_LOC_RIB case */
                if (bmp->targets->afimon[afi][safi] & BMP_MON_POSTPOLICY
                    || bmp->targets->afimon[afi][safi] & BMP_MON_LOC_RIB) {
                        for (bpiter = bgp_dest_get_bgp_path_info(bn); bpiter;
@@ -1236,7 +1235,6 @@ static bool bmp_wrqueue_locrib(struct bmp *bmp, struct pullwr *pullwr)
                goto out;
        }
 
-
        switch (bmp->afistate[afi][safi]) {
        case BMP_AFI_INACTIVE:
        case BMP_AFI_NEEDSYNC:
@@ -1248,9 +1246,6 @@ static bool bmp_wrqueue_locrib(struct bmp *bmp, struct pullwr *pullwr)
                         */
                        break;
 
-               /* TODO BMP_MON_LOCRIB check if this is true after implenting
-                * LOCRIB syncing
-                */
                /* currently syncing & haven't reached this prefix yet
                 * => it'll be sent as part of the table sync, no need here
                 */
@@ -1271,14 +1266,12 @@ static bool bmp_wrqueue_locrib(struct bmp *bmp, struct pullwr *pullwr)
                goto out;
        }
 
-       bn = bgp_node_lookup(bmp->targets->bgp->rib[afi][safi], &bqe->p);
-       struct prefix_rd *prd = NULL;
+       bool is_vpn = (bqe->afi == AFI_L2VPN && bqe->safi == SAFI_EVPN) ||
+                     (bqe->safi == SAFI_MPLS_VPN);
 
-       bool is_vpn = (bqe->afi == AFI_L2VPN && bqe->safi == SAFI_EVPN) || (bqe->safi == SAFI_MPLS_VPN);
-       if (is_vpn) {
-               prd = &bqe->rd;
-               bn = bgp_safi_node_lookup(bmp->targets->bgp->rib[afi][safi], afi, safi, &bqe->p, &bqe->rd);
-       }
+       struct prefix_rd *prd = is_vpn ? &bqe->rd : NULL;
+       bn = bgp_safi_node_lookup(bmp->targets->bgp->rib[afi][safi], safi,
+                                &bqe->p, prd);
 
        struct bgp_path_info *bpi;
 
@@ -1351,7 +1344,6 @@ static bool bmp_wrqueue(struct bmp *bmp, struct pullwr *pullwr)
        bn = bgp_safi_node_lookup(bmp->targets->bgp->rib[afi][safi], safi,
                                  &bqe->p, prd);
 
-       /* TODO BMP add MON_BGP_LOC_RIB case */
        if (bmp->targets->afimon[afi][safi] & BMP_MON_POSTPOLICY) {
                struct bgp_path_info *bpi;
 
@@ -1427,7 +1419,7 @@ static void bmp_wrerr(struct bmp *bmp, struct pullwr *pullwr, bool eof)
        bmp_free(bmp);
 }
 
-static void bmp_process_one(struct bmp_targets *bt, struct bgp *bgp, afi_t afi,
+static struct bmp_queue_entry* bmp_process_one(struct bmp_targets *bt, struct bmp_qhash_head* updhash, struct bmp_qlist_head* updlist, struct bgp *bgp, afi_t afi,
                            safi_t safi, struct bgp_dest *bn, struct peer *peer)
 {
        struct bmp *bmp;
@@ -1449,26 +1441,26 @@ static void bmp_process_one(struct bmp_targets *bt, struct bgp *bgp, afi_t afi,
                prefix_copy(&bqeref.rd,
                            (struct prefix_rd *)bgp_dest_get_prefix(bn->pdest));
 
-       bqe = bmp_qhash_find(&bt->updhash, &bqeref);
+       bqe = bmp_qhash_find(updhash, &bqeref);
        if (bqe) {
                if (bqe->refcount >= refcount)
                        /* nothing to do here */
                        return;
 
-               bmp_qlist_del(&bt->updlist, bqe);
+               bmp_qlist_del(updlist, bqe);
        } else {
                bqe = XMALLOC(MTYPE_BMP_QUEUE, sizeof(*bqe));
                memcpy(bqe, &bqeref, sizeof(*bqe));
 
-               bmp_qhash_add(&bt->updhash, bqe);
+               bmp_qhash_add(updhash, bqe);
        }
 
        bqe->refcount = refcount;
-       bmp_qlist_add_tail(&bt->updlist, bqe);
+       bmp_qlist_add_tail(updlist, bqe);
+
+       return bqe;
 
-       frr_each (bmp_session, &bt->sessions, bmp)
-               if (!bmp->queuepos)
-                       bmp->queuepos = bqe;
+       /* need to update correct queue pos for all sessions of the target after a call to this function */
 }
 
 static int bmp_process(struct bgp *bgp, afi_t afi, safi_t safi,
@@ -1490,12 +1482,16 @@ static int bmp_process(struct bgp *bgp, afi_t afi, safi_t safi,
                return 0;
 
        frr_each(bmp_targets, &bmpbgp->targets, bt) {
-               if (!bt->afimon[afi][safi])
+               /* check if any monitoring is enabled (ignoring loc-rib since it uses another hook & queue */
+               if (!(bt->afimon[afi][safi] & ~BMP_MON_LOC_RIB))
                        continue;
 
-               bmp_process_one(bt, bgp, afi, safi, bn, peer);
+               struct bmp_queue_entry* last_item = bmp_process_one(bt, &bt->updhash, &bt->updlist, bgp, afi, safi, bn, peer);
 
                frr_each(bmp_session, &bt->sessions, bmp) {
+                       if (!bmp->queuepos)
+                               bmp->queuepos = last_item;
+
                        pullwr_bump(bmp->pullwr);
                }
        }
@@ -2388,8 +2384,8 @@ DEFPY(bmp_stats_cfg,
        return CMD_SUCCESS;
 }
 
-DEFPY(bmp_monitor_cfg, bmp_monitor_cmd,
-      /* TODO BMP add loc-rib option */
+DEFPY(bmp_monitor_cfg,
+      bmp_monitor_cmd,
       "[no] bmp monitor <ipv4|ipv6|l2vpn> <unicast|multicast|evpn|vpn> <pre-policy|post-policy|loc-rib>$policy",
       NO_STR BMP_STR
       "Send BMP route monitoring messages\n" BGP_AF_STR BGP_AF_STR BGP_AF_STR
@@ -2409,11 +2405,8 @@ DEFPY(bmp_monitor_cfg, bmp_monitor_cmd,
        argv_find_and_parse_afi(argv, argc, &index, &afi);
        argv_find_and_parse_safi(argv, argc, &index, &safi);
 
-       /* TODO BMP set right flag */
        if (policy[0] == 'l') {
                flag = BMP_MON_LOC_RIB;
-               vty_out(vty,
-                       "changing loc rib monitoring config for this target\n");
        } else if (policy[1] == 'r')
                flag = BMP_MON_PREPOLICY;
        else
@@ -2762,48 +2755,11 @@ static int bmp_route_update(struct bgp *bgp, afi_t afi, safi_t safi,
        frr_each(bmp_targets, &bmpbgp->targets, bt) {
                if (bt->afimon[afi][safi] & BMP_MON_LOC_RIB) {
 
-                       struct bmp_queue_entry *bqe, bqeref;
-                       size_t refcount;
-
-                       refcount = bmp_session_count(&bt->sessions);
-                       if (refcount == 0)
-                               return 0;
-
-                       memset(&bqeref, 0, sizeof(bqeref));
-                       prefix_copy(&bqeref.p, prefix);
-                       bqeref.peerid = peer->qobj_node.nid;
-                       bqeref.afi = afi;
-                       bqeref.safi = safi;
-
-                       if ((afi == AFI_L2VPN && safi == SAFI_EVPN &&
-                            bn->pdest) ||
-                           (safi == SAFI_MPLS_VPN))
-                               prefix_copy(
-                                       &bqeref.rd,
-                                       (struct prefix_rd *)bgp_dest_get_prefix(
-                                               bn->pdest));
-
-                       bqe = bmp_qhash_find(&bt->locupdhash, &bqeref);
-                       uint32_t key = bmp_qhash_hkey(&bqeref);
-
-                       if (bqe) {
-                               if (bqe->refcount >= refcount)
-                                       /* nothing to do here */
-                                       return 0;
-
-                               bmp_qlist_del(&bt->locupdlist, bqe);
-                       } else {
-                               bqe = XMALLOC(MTYPE_BMP_QUEUE, sizeof(*bqe));
-                               memcpy(bqe, &bqeref, sizeof(*bqe));
-                               bmp_qhash_add(&bt->locupdhash, bqe);
-                       }
-
-                       bqe->refcount = refcount;
-                       bmp_qlist_add_tail(&bt->locupdlist, bqe);
+                       struct bmp_queue_entry *last_item = bmp_process_one(bt, &bt->locupdhash, &bt->locupdlist, bgp, afi, safi, bn, peer);
 
                        frr_each (bmp_session, &bt->sessions, bmp) {
                                if (!bmp->locrib_queuepos)
-                                       bmp->locrib_queuepos = bqe;
+                                       bmp->locrib_queuepos = last_item;
 
                                pullwr_bump(bmp->pullwr);
                        };

diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index 9353b64f305b06cbf1df9a54a97e1528953b1f4e..2dd84d457ab33e28d9f05136210646577aca477e 100644 (file)
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -3453,10 +3453,8 @@ static void bgp_process_main_one(struct bgp *bgp, struct bgp_dest *dest,
                UNSET_FLAG(new_select->flags, BGP_PATH_LINK_BW_CHG);
        }
 
+       /* call bmp hook for loc-rib route update / withdraw after flags were set */
        if (old_select || new_select) {
-               zlog_info("old_select==NULL %s | new_select==NULL %s",
-                         old_select == NULL ? "YES" : "NO",
-                         new_select == NULL ? "YES" : "NO");
 
                if (old_select) /* route is not installed in locrib anymore */
                        old_select->rib_uptime = (time_t)(-1L);