From 68e1a55bb151f398104f43aefc66f106f6b26bae Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 16 Nov 2017 19:41:58 -0500 Subject: [PATCH 1/3] bgpd: Only create json for aspath if needed The creation of the json object for the aspath is both memory intensive and expensive to create. Only create the json object when it is needed and stash it for further usage at that point. Signed-off-by: Donald Sharp --- bgpd/bgp_aspath.c | 80 +++++++++++++++++++++++++++-------------------- bgpd/bgp_aspath.h | 1 + bgpd/bgp_route.c | 2 ++ 3 files changed, 49 insertions(+), 34 deletions(-) diff --git a/bgpd/bgp_aspath.c b/bgpd/bgp_aspath.c index 6c03ba3059..caac385fb5 100644 --- a/bgpd/bgp_aspath.c +++ b/bgpd/bgp_aspath.c @@ -479,7 +479,7 @@ unsigned int aspath_has_as4(struct aspath *aspath) } /* Convert aspath structure to string expression. */ -static void aspath_make_str_count(struct aspath *as) +static void aspath_make_str_count(struct aspath *as, bool make_json) { struct assegment *seg; int str_size; @@ -489,14 +489,18 @@ static void aspath_make_str_count(struct aspath *as) json_object *jseg = NULL; json_object *jseg_list = NULL; - as->json = json_object_new_object(); - jaspath_segments = json_object_new_array(); + if (make_json) { + as->json = json_object_new_object(); + jaspath_segments = json_object_new_array(); + } /* Empty aspath. */ if (!as->segments) { - json_object_string_add(as->json, "string", "Local"); - json_object_object_add(as->json, "segments", jaspath_segments); - json_object_int_add(as->json, "length", 0); + if (make_json) { + json_object_string_add(as->json, "string", "Local"); + json_object_object_add(as->json, "segments", jaspath_segments); + json_object_int_add(as->json, "length", 0); + } as->str = XMALLOC(MTYPE_AS_STR, 1); as->str[0] = '\0'; as->str_len = 0; @@ -539,6 +543,7 @@ static void aspath_make_str_count(struct aspath *as) as->str_len = 0; json_object_free(as->json); as->json = NULL; + return; } @@ -564,12 +569,14 @@ static void aspath_make_str_count(struct aspath *as) str_buf + len, str_size - len, "%c", aspath_delimiter_char(seg->type, AS_SEG_START)); - jseg_list = json_object_new_array(); + if (make_json) + jseg_list = json_object_new_array(); /* write out the ASNs, with their seperators, bar the last one*/ for (i = 0; i < seg->length; i++) { - json_object_array_add(jseg_list, - json_object_new_int(seg->as[i])); + if (make_json) + json_object_array_add(jseg_list, + json_object_new_int(seg->as[i])); len += snprintf(str_buf + len, str_size - len, "%u", seg->as[i]); @@ -579,11 +586,13 @@ static void aspath_make_str_count(struct aspath *as) "%c", seperator); } - jseg = json_object_new_object(); - json_object_string_add(jseg, "type", - aspath_segment_type_str[seg->type]); - json_object_object_add(jseg, "list", jseg_list); - json_object_array_add(jaspath_segments, jseg); + if (make_json) { + jseg = json_object_new_object(); + json_object_string_add(jseg, "type", + aspath_segment_type_str[seg->type]); + json_object_object_add(jseg, "list", jseg_list); + json_object_array_add(jaspath_segments, jseg); + } if (seg->type != AS_SEQUENCE) len += snprintf( @@ -601,13 +610,16 @@ static void aspath_make_str_count(struct aspath *as) as->str = str_buf; as->str_len = len; - json_object_string_add(as->json, "string", str_buf); - json_object_object_add(as->json, "segments", jaspath_segments); - json_object_int_add(as->json, "length", aspath_count_hops(as)); + if (make_json) { + json_object_string_add(as->json, "string", str_buf); + json_object_object_add(as->json, "segments", jaspath_segments); + json_object_int_add(as->json, "length", aspath_count_hops(as)); + } + return; } -static void aspath_str_update(struct aspath *as) +void aspath_str_update(struct aspath *as, bool make_json) { if (as->str) XFREE(MTYPE_AS_STR, as->str); @@ -617,7 +629,7 @@ static void aspath_str_update(struct aspath *as) as->json = NULL; } - aspath_make_str_count(as); + aspath_make_str_count(as, make_json); } /* Intern allocated AS path. */ @@ -1079,7 +1091,7 @@ struct aspath *aspath_aggregate(struct aspath *as1, struct aspath *as2) } assegment_normalise(aspath->segments); - aspath_str_update(aspath); + aspath_str_update(aspath, false); return aspath; } @@ -1214,7 +1226,7 @@ struct aspath *aspath_replace_specific_asn(struct aspath *aspath, seg = seg->next; } - aspath_str_update(new); + aspath_str_update(new, false); return new; } @@ -1237,7 +1249,7 @@ struct aspath *aspath_replace_private_asns(struct aspath *aspath, as_t asn) seg = seg->next; } - aspath_str_update(new); + aspath_str_update(new, false); return new; } @@ -1307,7 +1319,7 @@ struct aspath *aspath_remove_private_asns(struct aspath *aspath) seg = seg->next; } - aspath_str_update(new); + aspath_str_update(new, false); return new; } @@ -1362,7 +1374,7 @@ static struct aspath *aspath_merge(struct aspath *as1, struct aspath *as2) last->next = as2->segments; as2->segments = new; - aspath_str_update(as2); + aspath_str_update(as2, false); return as2; } @@ -1381,7 +1393,7 @@ struct aspath *aspath_prepend(struct aspath *as1, struct aspath *as2) /* If as2 is empty, only need to dupe as1's chain onto as2 */ if (seg2 == NULL) { as2->segments = assegment_dup_all(as1->segments); - aspath_str_update(as2); + aspath_str_update(as2, false); return as2; } @@ -1432,7 +1444,7 @@ struct aspath *aspath_prepend(struct aspath *as1, struct aspath *as2) /* we've now prepended as1's segment chain to as2, merging * the inbetween AS_SEQUENCE of seg2 in the process */ - aspath_str_update(as2); + aspath_str_update(as2, false); return as2; } else { /* AS_SET merge code is needed at here. */ @@ -1511,7 +1523,7 @@ struct aspath *aspath_filter_exclude(struct aspath *source, lastseg->next = newseg; lastseg = newseg; } - aspath_str_update(newpath); + aspath_str_update(newpath, false); /* We are happy returning even an empty AS_PATH, because the * administrator * might expect this very behaviour. There's a mean to avoid this, if @@ -1549,7 +1561,7 @@ static struct aspath *aspath_add_asns(struct aspath *aspath, as_t asno, aspath->segments = newsegment; } - aspath_str_update(aspath); + aspath_str_update(aspath, false); return aspath; } @@ -1639,7 +1651,7 @@ struct aspath *aspath_reconcile_as4(struct aspath *aspath, if (!hops) { newpath = aspath_dup(as4path); - aspath_str_update(newpath); + aspath_str_update(newpath, false); return newpath; } @@ -1701,7 +1713,7 @@ struct aspath *aspath_reconcile_as4(struct aspath *aspath, mergedpath = aspath_merge(newpath, aspath_dup(as4path)); aspath_free(newpath); mergedpath->segments = assegment_normalise(mergedpath->segments); - aspath_str_update(mergedpath); + aspath_str_update(mergedpath, false); if (BGP_DEBUG(as4, AS4)) zlog_debug("[AS4] result of synthesizing is %s", @@ -1773,7 +1785,7 @@ struct aspath *aspath_delete_confed_seq(struct aspath *aspath) } if (removed_confed_segment) - aspath_str_update(aspath); + aspath_str_update(aspath, false); return aspath; } @@ -1824,7 +1836,7 @@ struct aspath *aspath_empty_get(void) struct aspath *aspath; aspath = aspath_new(); - aspath_make_str_count(aspath); + aspath_make_str_count(aspath, false); return aspath; } @@ -1975,7 +1987,7 @@ struct aspath *aspath_str2aspath(const char *str) } } - aspath_make_str_count(aspath); + aspath_make_str_count(aspath, false); return aspath; } @@ -1987,7 +1999,7 @@ unsigned int aspath_key_make(void *p) unsigned int key = 0; if (!aspath->str) - aspath_str_update(aspath); + aspath_str_update(aspath, false); key = jhash(aspath->str, aspath->str_len, 2334325); diff --git a/bgpd/bgp_aspath.h b/bgpd/bgp_aspath.h index f085cf3cb9..0c065cc936 100644 --- a/bgpd/bgp_aspath.h +++ b/bgpd/bgp_aspath.h @@ -92,6 +92,7 @@ extern struct aspath *aspath_delete_confed_seq(struct aspath *); extern struct aspath *aspath_empty(void); extern struct aspath *aspath_empty_get(void); extern struct aspath *aspath_str2aspath(const char *); +extern void aspath_str_update(struct aspath *as, bool make_json); extern void aspath_free(struct aspath *); extern struct aspath *aspath_intern(struct aspath *); extern void aspath_unintern(struct aspath **); diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 4ee1aafbe9..f0d91c366b 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -7395,6 +7395,8 @@ void route_vty_out_detail(struct vty *vty, struct bgp *bgp, struct prefix *p, /* Line1 display AS-path, Aggregator */ if (attr->aspath) { if (json_paths) { + if (!attr->aspath->json) + aspath_str_update(attr->aspath, true); json_object_lock(attr->aspath->json); json_object_object_add(json_path, "aspath", attr->aspath->json); From a69ea8aeac9d6cc5c27c084d02a0ddcd96231e46 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 16 Nov 2017 20:43:56 -0500 Subject: [PATCH 2/3] bgpd: Only build json for community when needed Building a communities json object every time is both expensive and memory wasteful. Modify code to only build the json object when needed. Signed-off-by: Donald Sharp --- bgpd/bgp_clist.c | 2 +- bgpd/bgp_community.c | 66 +++++++++++++++++++++++++++++--------------- bgpd/bgp_community.h | 2 +- bgpd/bgp_debug.c | 3 +- bgpd/bgp_route.c | 3 ++ bgpd/bgp_routemap.c | 2 +- bgpd/bgp_vty.c | 6 ++-- 7 files changed, 54 insertions(+), 30 deletions(-) diff --git a/bgpd/bgp_clist.c b/bgpd/bgp_clist.c index f3bae9535c..72b1098ede 100644 --- a/bgpd/bgp_clist.c +++ b/bgpd/bgp_clist.c @@ -438,7 +438,7 @@ static int community_regexp_match(struct community *com, regex_t *reg) if (com == NULL || com->size == 0) str = ""; else - str = community_str(com); + str = community_str(com, false); /* Regular expression match. */ if (regexec(reg, str, 0, NULL, 0) == 0) diff --git a/bgpd/bgp_community.c b/bgpd/bgp_community.c index b0f00d67d6..7c83eaa091 100644 --- a/bgpd/bgp_community.c +++ b/bgpd/bgp_community.c @@ -169,7 +169,6 @@ struct community *community_uniq_sort(struct community *com) return NULL; new = community_new(); - ; new->json = NULL; for (i = 0; i < com->size; i++) { @@ -195,7 +194,7 @@ struct community *community_uniq_sort(struct community *com) 0xFFFF0000 "graceful-shutdown" For other values, "AS:VAL" format is used. */ -static void set_community_string(struct community *com) +static void set_community_string(struct community *com, bool make_json) { int i; char *str; @@ -211,16 +210,20 @@ static void set_community_string(struct community *com) if (!com) return; - com->json = json_object_new_object(); - json_community_list = json_object_new_array(); + if (make_json) { + com->json = json_object_new_object(); + json_community_list = json_object_new_array(); + } /* When communities attribute is empty. */ if (com->size == 0) { str = XMALLOC(MTYPE_COMMUNITY_STR, 1); str[0] = '\0'; - json_object_string_add(com->json, "string", ""); - json_object_object_add(com->json, "list", json_community_list); + if (make_json) { + json_object_string_add(com->json, "string", ""); + json_object_object_add(com->json, "list", json_community_list); + } com->str = str; return; } @@ -273,47 +276,61 @@ static void set_community_string(struct community *com) case COMMUNITY_INTERNET: strcpy(pnt, "internet"); pnt += strlen("internet"); - json_string = json_object_new_string("internet"); - json_object_array_add(json_community_list, json_string); + if (make_json) { + json_string = json_object_new_string("internet"); + json_object_array_add(json_community_list, json_string); + } break; case COMMUNITY_NO_EXPORT: strcpy(pnt, "no-export"); pnt += strlen("no-export"); - json_string = json_object_new_string("noExport"); - json_object_array_add(json_community_list, json_string); + if (make_json) { + json_string = json_object_new_string("noExport"); + json_object_array_add(json_community_list, json_string); + } break; case COMMUNITY_NO_ADVERTISE: strcpy(pnt, "no-advertise"); pnt += strlen("no-advertise"); - json_string = json_object_new_string("noAdvertise"); - json_object_array_add(json_community_list, json_string); + if (make_json) { + json_string = json_object_new_string("noAdvertise"); + json_object_array_add(json_community_list, json_string); + } break; case COMMUNITY_LOCAL_AS: strcpy(pnt, "local-AS"); pnt += strlen("local-AS"); - json_string = json_object_new_string("localAs"); - json_object_array_add(json_community_list, json_string); + if (make_json) { + json_string = json_object_new_string("localAs"); + json_object_array_add(json_community_list, json_string); + } break; case COMMUNITY_GSHUT: strcpy(pnt, "graceful-shutdown"); pnt += strlen("graceful-shutdown"); - json_string = json_object_new_string("gracefulShutdown"); - json_object_array_add(json_community_list, json_string); + if (make_json) { + json_string = json_object_new_string("gracefulShutdown"); + json_object_array_add(json_community_list, json_string); + } break; default: as = (comval >> 16) & 0xFFFF; val = comval & 0xFFFF; sprintf(pnt, "%u:%d", as, val); - json_string = json_object_new_string(pnt); - json_object_array_add(json_community_list, json_string); + if (make_json) { + json_string = json_object_new_string(pnt); + json_object_array_add(json_community_list, json_string); + } pnt += strlen(pnt); break; } } *pnt = '\0'; - json_object_string_add(com->json, "string", str); - json_object_object_add(com->json, "list", json_community_list); + if (make_json) { + json_object_string_add(com->json, "string", str); + json_object_object_add(com->json, "list", json_community_list); + } com->str = str; } @@ -338,7 +355,7 @@ struct community *community_intern(struct community *com) /* Make string. */ if (!find->str) - set_community_string(find); + set_community_string(find, false); return find; } @@ -396,13 +413,16 @@ struct community *community_dup(struct community *com) } /* Retrun string representation of communities attribute. */ -char *community_str(struct community *com) +char *community_str(struct community *com, bool make_json) { if (!com) return NULL; + if (make_json && !com->json && com->str) + XFREE(MTYPE_COMMUNITY_STR, com->str); + if (!com->str) - set_community_string(com); + set_community_string(com, make_json); return com->str; } diff --git a/bgpd/bgp_community.h b/bgpd/bgp_community.h index f728debdb5..5016f132f2 100644 --- a/bgpd/bgp_community.h +++ b/bgpd/bgp_community.h @@ -63,7 +63,7 @@ extern struct community *community_uniq_sort(struct community *); extern struct community *community_parse(u_int32_t *, u_short); extern struct community *community_intern(struct community *); extern void community_unintern(struct community **); -extern char *community_str(struct community *); +extern char *community_str(struct community *, bool make_json); extern unsigned int community_hash_make(struct community *); extern struct community *community_str2com(const char *); extern int community_match(const struct community *, const struct community *); diff --git a/bgpd/bgp_debug.c b/bgpd/bgp_debug.c index 6e16d5f45b..45ac8e6859 100644 --- a/bgpd/bgp_debug.c +++ b/bgpd/bgp_debug.c @@ -385,7 +385,8 @@ int bgp_dump_attr(struct attr *attr, char *buf, size_t size) if (CHECK_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_COMMUNITIES))) snprintf(buf + strlen(buf), size - strlen(buf), - ", community %s", community_str(attr->community)); + ", community %s", community_str(attr->community, + false)); if (CHECK_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_EXT_COMMUNITIES))) snprintf(buf + strlen(buf), size - strlen(buf), diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index f0d91c366b..ba75da4591 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -7885,6 +7885,9 @@ void route_vty_out_detail(struct vty *vty, struct bgp *bgp, struct prefix *p, /* Line 4 display Community */ if (attr->community) { if (json_paths) { + if (!attr->community->json) + community_str(attr->community, + true); json_object_lock(attr->community->json); json_object_object_add(json_path, "community", attr->community->json); diff --git a/bgpd/bgp_routemap.c b/bgpd/bgp_routemap.c index 45487aa003..cc8cfd550c 100644 --- a/bgpd/bgp_routemap.c +++ b/bgpd/bgp_routemap.c @@ -3813,7 +3813,7 @@ DEFUN (set_community, } /* Set communites attribute string. */ - str = community_str(com); + str = community_str(com, false); if (additive) { argstr = XCALLOC(MTYPE_TMP, diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index db19835f88..40f706a646 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -10010,7 +10010,7 @@ static void community_show_all_iterator(struct hash_backet *backet, com = (struct community *)backet->data; vty_out(vty, "[%p] (%ld) %s\n", (void *)com, com->refcnt, - community_str(com)); + community_str(com, false)); } /* Show BGP's community internal data. */ @@ -12698,7 +12698,7 @@ static void community_list_show(struct vty *vty, struct community_list *list) vty_out(vty, " %s %s\n", community_direct_str(entry->direct), entry->style == COMMUNITY_LIST_STANDARD - ? community_str(entry->u.com) + ? community_str(entry->u.com, false) : entry->config); } } @@ -13354,7 +13354,7 @@ static const char *community_list_config_str(struct community_entry *entry) str = ""; else { if (entry->style == COMMUNITY_LIST_STANDARD) - str = community_str(entry->u.com); + str = community_str(entry->u.com, false); else str = entry->config; } From 8cc845631720964071375b7c121c0c084f9f5564 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Fri, 17 Nov 2017 12:53:54 -0500 Subject: [PATCH 3/3] bgpd: Fix json output in some situations This commit fixes a bug where json output would display ',,,,,,,' because we were deciding to not display information about some routes due to a selection criteria. Signed-off-by: Donald Sharp --- bgpd/bgp_route.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index ba75da4591..a1c891fc66 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -8187,8 +8187,6 @@ static int bgp_show_table(struct vty *vty, struct bgp *bgp, safi_t safi, continue; display = 0; - if (!first && use_json) - vty_out(vty, ","); if (use_json) json_paths = json_object_new_array(); else @@ -8384,7 +8382,11 @@ static int bgp_show_table(struct vty *vty, struct bgp *bgp, safi_t safi, inet_ntop(p->family, &p->u.prefix, buf, BUFSIZ), p->prefixlen); - vty_out(vty, "\"%s\": ", buf2); + if (first) + vty_out(vty, "\"%s\": ", buf2); + else + vty_out(vty, ",\"%s\": ", buf2); + vty_out(vty, "%s", json_object_to_json_string_ext(json_paths, JSON_C_TO_STRING_PRETTY)); json_object_free(json_paths);