Merge pull request #1118 from opensourcerouting/attr-kill-master

kill bgp attr badness

bgpd/bgp_attr.c

@@ -161,22 +161,6 @@ static void cluster_free(struct cluster_list *cluster)
 	XFREE(MTYPE_CLUSTER, cluster);
 }
 
-static struct cluster_list *cluster_dup(struct cluster_list *cluster)
-{
-	struct cluster_list *new;
-
-	new = XCALLOC(MTYPE_CLUSTER, sizeof(struct cluster_list));
-	new->length = cluster->length;
-
-	if (cluster->length) {
-		new->list = XMALLOC(MTYPE_CLUSTER_VAL, cluster->length);
-		memcpy(new->list, cluster->list, cluster->length);
-	} else
-		new->list = NULL;
-
-	return new;
-}
-
 static struct cluster_list *cluster_intern(struct cluster_list *cluster)
 {
 	struct cluster_list *find;
@@ -422,21 +406,6 @@ static void transit_free(struct transit *transit)
 	XFREE(MTYPE_TRANSIT, transit);
 }
 
-static struct transit *transit_dup(struct transit *transit)
-{
-	struct transit *new;
-
-	new = XCALLOC(MTYPE_TRANSIT, sizeof(struct transit));
-	new->length = transit->length;
-	if (new->length) {
-		new->val = XMALLOC(MTYPE_TRANSIT_VAL, transit->length);
-		memcpy(new->val, transit->val, transit->length);
-	} else
-		new->val = NULL;
-
-	return new;
-}
-
 static void *transit_hash_alloc(void *p)
 {
 	/* Transit structure is already allocated.  */
@@ -508,50 +477,6 @@ void bgp_attr_dup(struct attr *new, struct attr *orig)
 	*new = *orig;
 }
 
-void bgp_attr_deep_dup(struct attr *new, struct attr *orig)
-{
-	if (orig->aspath)
-		new->aspath = aspath_dup(orig->aspath);
-
-	if (orig->community)
-		new->community = community_dup(orig->community);
-
-	if (orig->ecommunity)
-		new->ecommunity = ecommunity_dup(orig->ecommunity);
-	if (orig->cluster)
-		new->cluster = cluster_dup(orig->cluster);
-	if (orig->transit)
-		new->transit = transit_dup(orig->transit);
-	if (orig->encap_subtlvs)
-		new->encap_subtlvs = encap_tlv_dup(orig->encap_subtlvs);
-#if ENABLE_BGP_VNC
-	if (orig->vnc_subtlvs)
-		new->vnc_subtlvs = encap_tlv_dup(orig->vnc_subtlvs);
-#endif
-}
-
-void bgp_attr_deep_free(struct attr *attr)
-{
-	if (attr->aspath)
-		aspath_free(attr->aspath);
-
-	if (attr->community)
-		community_free(attr->community);
-
-	if (attr->ecommunity)
-		ecommunity_free(&attr->ecommunity);
-	if (attr->cluster)
-		cluster_free(attr->cluster);
-	if (attr->transit)
-		transit_free(attr->transit);
-	if (attr->encap_subtlvs)
-		encap_free(attr->encap_subtlvs);
-#if ENABLE_BGP_VNC
-	if (attr->vnc_subtlvs)
-		encap_free(attr->vnc_subtlvs);
-#endif
-}
-
 unsigned long int attr_count(void)
 {
 	return attrhash->count;
@@ -765,51 +690,18 @@ struct attr *bgp_attr_intern(struct attr *attr)
 	}
 #endif
 
+	/* At this point, attr only contains intern'd pointers.  that means
+	 * if we find it in attrhash, it has all the same pointers and we
+	 * correctly updated the refcounts on these.
+	 * If we don't find it, we need to allocate a one because in all
+	 * cases this returns a new reference to a hashed attr, but the input
+	 * wasn't on hash. */
 	find = (struct attr *)hash_get(attrhash, attr, bgp_attr_hash_alloc);
 	find->refcnt++;
 
 	return find;
 }
 
-/**
- * Increment the refcount on various structures that attr holds.
- * Note on usage: call _only_ when the 'attr' object has already
- * been 'intern'ed and exists in 'attrhash' table. The function
- * serves to hold a reference to that (real) object.
- * Note also that the caller can safely call bgp_attr_unintern()
- * after calling bgp_attr_refcount(). That would release the
- * reference and could result in a free() of the attr object.
- */
-struct attr *bgp_attr_refcount(struct attr *attr)
-{
-	/* Intern referenced strucutre. */
-	if (attr->aspath)
-		attr->aspath->refcnt++;
-
-	if (attr->community)
-		attr->community->refcnt++;
-
-	if (attr->ecommunity)
-		attr->ecommunity->refcnt++;
-
-	if (attr->cluster)
-		attr->cluster->refcnt++;
-
-	if (attr->transit)
-		attr->transit->refcnt++;
-
-	if (attr->encap_subtlvs)
-		attr->encap_subtlvs->refcnt++;
-
-#if ENABLE_BGP_VNC
-	if (attr->vnc_subtlvs)
-		attr->vnc_subtlvs->refcnt++;
-#endif
-
-	attr->refcnt++;
-	return attr;
-}
-
 /* Make network statement's attribute. */
 struct attr *bgp_attr_default_set(struct attr *attr, u_char origin)
 {

bgpd/bgp_attr.h

@@ -207,7 +207,12 @@ struct transit {
 	u_char *val;
 };
 
-#define ATTR_FLAG_BIT(X)  (1ULL << ((X) - 1))
+/* "(void) 0" will generate a compiler error.  this is a safety check to
+ * ensure we're not using a value that exceeds the bit size of attr->flag. */
+#define ATTR_FLAG_BIT(X) \
+	__builtin_choose_expr((X) >= 1 && (X) <= 64, \
+			      1ULL << ((X) - 1), \
+			      (void) 0)
 
 #define BGP_CLUSTER_LIST_LENGTH(attr)                                          \
 	(((attr)->flag & ATTR_FLAG_BIT(BGP_ATTR_CLUSTER_LIST))                 \
@@ -233,10 +238,7 @@ extern bgp_attr_parse_ret_t bgp_attr_parse(struct peer *, struct attr *,
 					   bgp_size_t, struct bgp_nlri *,
 					   struct bgp_nlri *);
 extern void bgp_attr_dup(struct attr *, struct attr *);
-extern void bgp_attr_deep_dup(struct attr *, struct attr *);
-extern void bgp_attr_deep_free(struct attr *);
 extern struct attr *bgp_attr_intern(struct attr *attr);
-extern struct attr *bgp_attr_refcount(struct attr *attr);
 extern void bgp_attr_unintern_sub(struct attr *);
 extern void bgp_attr_unintern(struct attr **);
 extern void bgp_attr_flush(struct attr *);

bgpd/bgp_route.c

@@ -6339,7 +6339,7 @@ void bgp_redistribute_withdraw(struct bgp *bgp, afi_t afi, int type,
 static void route_vty_out_route(struct prefix *p, struct vty *vty,
 				json_object *json)
 {
-	int len;
+	int len = 0;
 	u_int32_t destination;
 	char buf[BUFSIZ];
 

bgpd/bgp_updgrp.c

@@ -1185,7 +1185,7 @@ static void update_subgroup_copy_adj_out(struct update_subgroup *source,
 		aout_copy =
 			bgp_adj_out_alloc(dest, aout->rn, aout->addpath_tx_id);
 		aout_copy->attr =
-			aout->attr ? bgp_attr_refcount(aout->attr) : NULL;
+			aout->attr ? bgp_attr_intern(aout->attr) : NULL;
 	}
 }
 

bgpd/bgp_zebra.c

@@ -58,41 +58,6 @@
 /* All information about zebra. */
 struct zclient *zclient = NULL;
 
-/* These array buffers are used in making a copy of the attributes for
-   route-map apply. Arrays are being used here to minimize mallocs and
-   frees for the temporary copy of the attributes.
-   Given the zapi api expects the nexthop buffer to contain pointer to
-   pointers for nexthops, we couldnt have used a single nexthop variable
-   on the stack, hence we had two options:
-     1. maintain a linked-list and free it after zapi_*_route call
-     2. use an array to avoid number of mallocs.
-   Number of supported next-hops are finite, use of arrays should be ok. */
-struct attr attr_cp[MULTIPATH_NUM];
-unsigned int attr_index = 0;
-
-/* Once per address-family initialization of the attribute array */
-#define BGP_INFO_ATTR_BUF_INIT()                                               \
-	do {                                                                   \
-		memset(attr_cp, 0, MULTIPATH_NUM * sizeof(struct attr));       \
-		attr_index = 0;                                                \
-	} while (0)
-
-#define BGP_INFO_ATTR_BUF_COPY(info_src, info_dst)                             \
-	do {                                                                   \
-		*info_dst = *info_src;                                         \
-		assert(attr_index != multipath_num);                           \
-		bgp_attr_dup(&attr_cp[attr_index], info_src->attr);            \
-		bgp_attr_deep_dup(&attr_cp[attr_index], info_src->attr);       \
-		info_dst->attr = &attr_cp[attr_index];                         \
-		attr_index++;                                                  \
-	} while (0)
-
-#define BGP_INFO_ATTR_BUF_FREE(info)                                           \
-	do {                                                                   \
-		bgp_attr_deep_free(info->attr);                                \
-	} while (0)
-
-
 /* Can we install into zebra? */
 static inline int bgp_install_info_to_zebra(struct bgp *bgp)
 {
@@ -950,7 +915,12 @@ static struct in6_addr *bgp_info_to_ipv6_nexthop(struct bgp_info *info)
 static int bgp_table_map_apply(struct route_map *map, struct prefix *p,
 			       struct bgp_info *info)
 {
-	if (route_map_apply(map, p, RMAP_BGP, info) != RMAP_DENYMATCH)
+	route_map_result_t ret;
+
+	ret = route_map_apply(map, p, RMAP_BGP, info);
+	bgp_attr_flush(info->attr);
+
+	if (ret != RMAP_DENYMATCH)
 		return 1;
 
 	if (bgp_debug_zebra(p)) {
@@ -992,8 +962,9 @@ void bgp_zebra_announce(struct bgp_node *rn, struct prefix *p,
 	struct peer *peer;
 	struct bgp_info *mpinfo;
 	u_int32_t metric;
+	struct attr local_attr;
 	struct bgp_info local_info;
-	struct bgp_info *info_cp = &local_info;
+	struct bgp_info *mpinfo_cp = &local_info;
 	route_tag_t tag;
 	mpls_label_t label;
 
@@ -1046,37 +1017,34 @@ void bgp_zebra_announce(struct bgp_node *rn, struct prefix *p,
 	else
 		return;
 
-	if (bgp->table_map[afi][safi].name)
-		BGP_INFO_ATTR_BUF_INIT();
-
 	/* Metric is currently based on the best-path only */
 	metric = info->attr->med;
 	for (mpinfo = info; mpinfo; mpinfo = bgp_info_mpath_next(mpinfo)) {
+		*mpinfo_cp = *mpinfo;
+
 		if (nh_family == AF_INET) {
 			struct in_addr *nexthop;
 
-			nexthop = NULL;
-
 			if (bgp->table_map[afi][safi].name) {
 				/* Copy info and attributes, so the route-map
-				   apply doesn't modify the
+				   apply doesn't modify the BGP route info. */
-				   BGP route info. */
+				local_attr = *mpinfo->attr;
-				BGP_INFO_ATTR_BUF_COPY(mpinfo, info_cp);
+				mpinfo_cp->attr = &local_attr;
-				if (bgp_table_map_apply(
-					    bgp->table_map[afi][safi].map, p,
-					    info_cp)) {
-					if (mpinfo == info) {
-						metric = info_cp->attr->med;
-						tag = info_cp->attr->tag;
-					}
-					nexthop = &info_cp->attr->nexthop;
-				}
-				BGP_INFO_ATTR_BUF_FREE(info_cp);
-			} else
-				nexthop = &mpinfo->attr->nexthop;
 
-			if (nexthop == NULL)
+				if (!bgp_table_map_apply(
-				continue;
+					    bgp->table_map[afi][safi].map, p,
+					    mpinfo_cp))
+					continue;
+
+				/* metric/tag is only allowed to be
+				 * overridden on 1st nexthop */
+				if (mpinfo == info) {
+					metric = mpinfo_cp->attr->med;
+					tag = mpinfo_cp->attr->tag;
+				}
+			}
+
+			nexthop = &mpinfo_cp->attr->nexthop;
 
 			api_nh = &api.nexthops[valid_nh_count];
 			api_nh->gate.ipv4 = *nexthop;
@@ -1086,29 +1054,26 @@ void bgp_zebra_announce(struct bgp_node *rn, struct prefix *p,
 			struct in6_addr *nexthop;
 
 			ifindex = 0;
-			nexthop = NULL;
 
 			if (bgp->table_map[afi][safi].name) {
 				/* Copy info and attributes, so the route-map
-				   apply doesn't modify the
+				   apply doesn't modify the BGP route info. */
-				   BGP route info. */
+				local_attr = *mpinfo->attr;
-				BGP_INFO_ATTR_BUF_COPY(mpinfo, info_cp);
+				mpinfo_cp->attr = &local_attr;
-				if (bgp_table_map_apply(
-					    bgp->table_map[afi][safi].map, p,
-					    info_cp)) {
-					if (mpinfo == info) {
-						metric = info_cp->attr->med;
-						tag = info_cp->attr->tag;
-					}
-					nexthop = bgp_info_to_ipv6_nexthop(
-						info_cp);
-				}
-				BGP_INFO_ATTR_BUF_FREE(info_cp);
-			} else
-				nexthop = bgp_info_to_ipv6_nexthop(mpinfo);
 
-			if (nexthop == NULL)
+				if (!bgp_table_map_apply(
-				continue;
+					    bgp->table_map[afi][safi].map, p,
+					    mpinfo_cp))
+					continue;
+
+				/* metric/tag is only allowed to be
+				 * overridden on 1st nexthop */
+				if (mpinfo == info) {
+					metric = mpinfo_cp->attr->med;
+					tag = mpinfo_cp->attr->tag;
+				}
+			}
+			nexthop = bgp_info_to_ipv6_nexthop(mpinfo_cp);
 
 			if ((mpinfo == info)
 			    && mpinfo->attr->mp_nexthop_len