From e61f7c0a106f501db64ed0622f66c228efcc2c14 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 5 Feb 2020 20:55:30 -0500 Subject: [PATCH 1/6] bgpd: show martian nexthops improve code flow The show martian nexthops command for bgp had some strangely duplicated code. Refactor. Signed-off-by: Donald Sharp --- bgpd/bgp_nexthop.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/bgpd/bgp_nexthop.c b/bgpd/bgp_nexthop.c index 7116c80941..1e565af894 100644 --- a/bgpd/bgp_nexthop.c +++ b/bgpd/bgp_nexthop.c @@ -192,17 +192,10 @@ static void show_address_entry(struct hash_bucket *bucket, void *args) struct listnode *node; char str[INET6_ADDRSTRLEN] = {0}; - if (addr->p->family == AF_INET) { - vty_out(vty, "addr: %s, count: %d : ", inet_ntop(AF_INET, - &(addr->p->u.prefix4), - str, INET_ADDRSTRLEN), - addr->ifp_name_list->count); - } else if (addr->p->family == AF_INET6) { - vty_out(vty, "addr: %s, count: %d : ", inet_ntop(AF_INET6, - &(addr->p->u.prefix6), - str, INET6_ADDRSTRLEN), - addr->ifp_name_list->count); - } + vty_out(vty, "addr: %s, count: %d : ", + inet_ntop(addr->p->family, &(addr->p->u.prefix), + str, INET6_ADDRSTRLEN), + addr->ifp_name_list->count); for (ALL_LIST_ELEMENTS_RO(addr->ifp_name_list, node, name)) { vty_out(vty, " %s,", name); From 2ec802d173ede4211d7f2b40b80bb36569c60504 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 5 Feb 2020 21:02:25 -0500 Subject: [PATCH 2/6] bgpd: Remove prefix pointer creation The creation of a prefix pointer is unnecessary. Save the prefix as part of the actual data structure. This will reduce the data needed by 8 bytes per nexthop stored. Signed-off-by: Donald Sharp --- bgpd/bgp_nexthop.c | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/bgpd/bgp_nexthop.c b/bgpd/bgp_nexthop.c index 1e565af894..896285676c 100644 --- a/bgpd/bgp_nexthop.c +++ b/bgpd/bgp_nexthop.c @@ -180,7 +180,7 @@ void bgp_tip_del(struct bgp *bgp, struct in_addr *tip) /* BGP own address structure */ struct bgp_addr { - struct prefix *p; + struct prefix p; struct list *ifp_name_list; }; @@ -193,7 +193,7 @@ static void show_address_entry(struct hash_bucket *bucket, void *args) char str[INET6_ADDRSTRLEN] = {0}; vty_out(vty, "addr: %s, count: %d : ", - inet_ntop(addr->p->family, &(addr->p->u.prefix), + inet_ntop(addr->p.family, &(addr->p.u.prefix), str, INET6_ADDRSTRLEN), addr->ifp_name_list->count); @@ -224,8 +224,7 @@ static void *bgp_address_hash_alloc(void *p) struct bgp_addr *addr = NULL; addr = XMALLOC(MTYPE_BGP_ADDR, sizeof(struct bgp_addr)); - addr->p = prefix_new(); - prefix_copy(addr->p, copy_addr->p); + prefix_copy(&addr->p, ©_addr->p); addr->ifp_name_list = list_new(); addr->ifp_name_list->del = bgp_address_hash_string_del; @@ -237,7 +236,6 @@ static void bgp_address_hash_free(void *data) { struct bgp_addr *addr = data; - prefix_free(&addr->p); list_delete(&addr->ifp_name_list); XFREE(MTYPE_BGP_ADDR, addr); } @@ -246,7 +244,7 @@ static unsigned int bgp_address_hash_key_make(const void *p) { const struct bgp_addr *addr = p; - return prefix_hash_key((const void *)(addr->p)); + return prefix_hash_key(&addr->p); } static bool bgp_address_hash_cmp(const void *p1, const void *p2) @@ -254,7 +252,7 @@ static bool bgp_address_hash_cmp(const void *p1, const void *p2) const struct bgp_addr *addr1 = p1; const struct bgp_addr *addr2 = p2; - return prefix_same(addr1->p, addr2->p); + return prefix_same(&addr1->p, &addr2->p); } void bgp_address_init(struct bgp *bgp) @@ -281,12 +279,12 @@ static void bgp_address_add(struct bgp *bgp, struct connected *ifc, struct listnode *node; char *name; - tmp.p = p; + tmp.p = *p; - if (tmp.p->family == AF_INET) - tmp.p->prefixlen = IPV4_MAX_BITLEN; - else if (tmp.p->family == AF_INET6) - tmp.p->prefixlen = IPV6_MAX_BITLEN; + if (tmp.p.family == AF_INET) + tmp.p.prefixlen = IPV4_MAX_BITLEN; + else if (tmp.p.family == AF_INET6) + tmp.p.prefixlen = IPV6_MAX_BITLEN; addr = hash_get(bgp->address_hash, &tmp, bgp_address_hash_alloc); @@ -308,12 +306,12 @@ static void bgp_address_del(struct bgp *bgp, struct connected *ifc, struct listnode *node; char *name; - tmp.p = p; + tmp.p = *p; - if (tmp.p->family == AF_INET) - tmp.p->prefixlen = IPV4_MAX_BITLEN; - else if (tmp.p->family == AF_INET6) - tmp.p->prefixlen = IPV6_MAX_BITLEN; + if (tmp.p.family == AF_INET) + tmp.p.prefixlen = IPV4_MAX_BITLEN; + else if (tmp.p.family == AF_INET6) + tmp.p.prefixlen = IPV6_MAX_BITLEN; addr = hash_lookup(bgp->address_hash, &tmp); /* may have been deleted earlier by bgp_interface_down() */ @@ -529,7 +527,7 @@ int bgp_nexthop_self(struct bgp *bgp, afi_t afi, uint8_t type, uint8_t sub_type, break; } - tmp_addr.p = &p; + tmp_addr.p = p; addr = hash_lookup(bgp->address_hash, &tmp_addr); if (addr) return 1; @@ -541,9 +539,8 @@ int bgp_nexthop_self(struct bgp *bgp, afi_t afi, uint8_t type, uint8_t sub_type, if (attr->flag & ATTR_FLAG_BIT(BGP_ATTR_NEXT_HOP)) { tmp_tip.addr = attr->nexthop; } else if ((attr->mp_nexthop_len) && - ((attr->mp_nexthop_len == BGP_ATTR_NHLEN_IPV4) - || (attr->mp_nexthop_len == - BGP_ATTR_NHLEN_VPNV4))) { + ((attr->mp_nexthop_len == BGP_ATTR_NHLEN_IPV4) + || (attr->mp_nexthop_len == BGP_ATTR_NHLEN_VPNV4))) { tmp_tip.addr = attr->mp_nexthop_global_in; } From af34d2da11139cab5ffea89d264eedb79495c2fd Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 5 Feb 2020 21:21:56 -0500 Subject: [PATCH 3/6] bgpd: bgp_nexthop_self optimize afi and new_afi handling The new_afi and afi were being used over and over. Switch to the end result we want and just use that from the get go. Signed-off-by: Donald Sharp --- bgpd/bgp_nexthop.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/bgpd/bgp_nexthop.c b/bgpd/bgp_nexthop.c index 896285676c..ed264a9665 100644 --- a/bgpd/bgp_nexthop.c +++ b/bgpd/bgp_nexthop.c @@ -474,7 +474,7 @@ int bgp_nexthop_self(struct bgp *bgp, afi_t afi, uint8_t type, uint8_t sub_type, struct attr *attr, struct bgp_node *rn) { struct prefix p = {0}; - afi_t new_afi = afi; + uint8_t new_afi = afi == AFI_IP ? AF_INET : AF_INET6; struct bgp_addr tmp_addr = {0}, *addr = NULL; struct tip_addr tmp_tip, *tip = NULL; @@ -484,11 +484,11 @@ int bgp_nexthop_self(struct bgp *bgp, afi_t afi, uint8_t type, uint8_t sub_type, : false; if (!is_bgp_static_route) - new_afi = BGP_ATTR_NEXTHOP_AFI_IP6(attr) ? AFI_IP6 : AFI_IP; + new_afi = BGP_ATTR_NEXTHOP_AFI_IP6(attr) ? AF_INET6 : AF_INET; + p.family = new_afi; switch (new_afi) { - case AFI_IP: - p.family = AF_INET; + case AF_INET: if (is_bgp_static_route) { p.u.prefix4 = rn->p.u.prefix4; p.prefixlen = rn->p.prefixlen; @@ -512,9 +512,7 @@ int bgp_nexthop_self(struct bgp *bgp, afi_t afi, uint8_t type, uint8_t sub_type, return 0; } break; - case AFI_IP6: - p.family = AF_INET6; - + case AF_INET6: if (is_bgp_static_route) { p.u.prefix6 = rn->p.u.prefix6; p.prefixlen = rn->p.prefixlen; @@ -532,7 +530,7 @@ int bgp_nexthop_self(struct bgp *bgp, afi_t afi, uint8_t type, uint8_t sub_type, if (addr) return 1; - if (new_afi == AFI_IP) { + if (new_afi == AF_INET) { memset(&tmp_tip, 0, sizeof(struct tip_addr)); tmp_tip.addr = attr->nexthop; From e26c305530618cacf8b64a52882683bd4dba573d Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 5 Feb 2020 21:30:03 -0500 Subject: [PATCH 4/6] bgpd: Store data in final temp variable There is no need to have a temp variable to then store that data in another temporary variable. Signed-off-by: Donald Sharp --- bgpd/bgp_nexthop.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/bgpd/bgp_nexthop.c b/bgpd/bgp_nexthop.c index ed264a9665..49111ff8c8 100644 --- a/bgpd/bgp_nexthop.c +++ b/bgpd/bgp_nexthop.c @@ -473,7 +473,6 @@ static void bgp_connected_cleanup(struct route_table *table, int bgp_nexthop_self(struct bgp *bgp, afi_t afi, uint8_t type, uint8_t sub_type, struct attr *attr, struct bgp_node *rn) { - struct prefix p = {0}; uint8_t new_afi = afi == AFI_IP ? AF_INET : AF_INET6; struct bgp_addr tmp_addr = {0}, *addr = NULL; struct tip_addr tmp_tip, *tip = NULL; @@ -486,46 +485,45 @@ int bgp_nexthop_self(struct bgp *bgp, afi_t afi, uint8_t type, uint8_t sub_type, if (!is_bgp_static_route) new_afi = BGP_ATTR_NEXTHOP_AFI_IP6(attr) ? AF_INET6 : AF_INET; - p.family = new_afi; + tmp_addr.p.family = new_afi; switch (new_afi) { case AF_INET: if (is_bgp_static_route) { - p.u.prefix4 = rn->p.u.prefix4; - p.prefixlen = rn->p.prefixlen; + tmp_addr.p.u.prefix4 = rn->p.u.prefix4; + tmp_addr.p.prefixlen = rn->p.prefixlen; } else { /* Here we need to find out which nexthop to be used*/ if (attr->flag & ATTR_FLAG_BIT(BGP_ATTR_NEXT_HOP)) { - p.u.prefix4 = attr->nexthop; - p.prefixlen = IPV4_MAX_BITLEN; + tmp_addr.p.u.prefix4 = attr->nexthop; + tmp_addr.p.prefixlen = IPV4_MAX_BITLEN; } else if ((attr->mp_nexthop_len) && ((attr->mp_nexthop_len == BGP_ATTR_NHLEN_IPV4) || (attr->mp_nexthop_len == BGP_ATTR_NHLEN_VPNV4))) { - p.u.prefix4 = + tmp_addr.p.u.prefix4 = attr->mp_nexthop_global_in; - p.prefixlen = IPV4_MAX_BITLEN; + tmp_addr.p.prefixlen = IPV4_MAX_BITLEN; } else return 0; } break; case AF_INET6: if (is_bgp_static_route) { - p.u.prefix6 = rn->p.u.prefix6; - p.prefixlen = rn->p.prefixlen; + tmp_addr.p.u.prefix6 = rn->p.u.prefix6; + tmp_addr.p.prefixlen = rn->p.prefixlen; } else { - p.u.prefix6 = attr->mp_nexthop_global; - p.prefixlen = IPV6_MAX_BITLEN; + tmp_addr.p.u.prefix6 = attr->mp_nexthop_global; + tmp_addr.p.prefixlen = IPV6_MAX_BITLEN; } break; default: break; } - tmp_addr.p = p; addr = hash_lookup(bgp->address_hash, &tmp_addr); if (addr) return 1; From c4fb2504916ceb8bc48c0937ce9a42e42af8f4b6 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 6 Feb 2020 07:17:01 -0500 Subject: [PATCH 5/6] bgpd: Fix up some poor formatting Signed-off-by: Donald Sharp --- bgpd/bgp_nexthop.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/bgpd/bgp_nexthop.c b/bgpd/bgp_nexthop.c index 49111ff8c8..41a5577a70 100644 --- a/bgpd/bgp_nexthop.c +++ b/bgpd/bgp_nexthop.c @@ -477,8 +477,8 @@ int bgp_nexthop_self(struct bgp *bgp, afi_t afi, uint8_t type, uint8_t sub_type, struct bgp_addr tmp_addr = {0}, *addr = NULL; struct tip_addr tmp_tip, *tip = NULL; - bool is_bgp_static_route = ((type == ZEBRA_ROUTE_BGP) - && (sub_type == BGP_ROUTE_STATIC)) + bool is_bgp_static_route = + ((type == ZEBRA_ROUTE_BGP) && (sub_type == BGP_ROUTE_STATIC)) ? true : false; @@ -493,17 +493,14 @@ int bgp_nexthop_self(struct bgp *bgp, afi_t afi, uint8_t type, uint8_t sub_type, tmp_addr.p.prefixlen = rn->p.prefixlen; } else { /* Here we need to find out which nexthop to be used*/ - if (attr->flag & - ATTR_FLAG_BIT(BGP_ATTR_NEXT_HOP)) { - + if (attr->flag & ATTR_FLAG_BIT(BGP_ATTR_NEXT_HOP)) { tmp_addr.p.u.prefix4 = attr->nexthop; tmp_addr.p.prefixlen = IPV4_MAX_BITLEN; - - } else if ((attr->mp_nexthop_len) && - ((attr->mp_nexthop_len == - BGP_ATTR_NHLEN_IPV4) || - (attr->mp_nexthop_len == - BGP_ATTR_NHLEN_VPNV4))) { + } else if ((attr->mp_nexthop_len) + && ((attr->mp_nexthop_len + == BGP_ATTR_NHLEN_IPV4) + || (attr->mp_nexthop_len + == BGP_ATTR_NHLEN_VPNV4))) { tmp_addr.p.u.prefix4 = attr->mp_nexthop_global_in; tmp_addr.p.prefixlen = IPV4_MAX_BITLEN; From 8c5c49ace83970ed002dc2f88df17f3239c4071c Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 6 Feb 2020 08:23:13 -0500 Subject: [PATCH 6/6] bgpd: Cleanup compile error? For some reason we are getting a compile error around a variable I didn't touch in the other commits. Make it happy. Signed-off-by: Donald Sharp --- bgpd/bgp_nexthop.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bgpd/bgp_nexthop.c b/bgpd/bgp_nexthop.c index 41a5577a70..ab0c3a3f11 100644 --- a/bgpd/bgp_nexthop.c +++ b/bgpd/bgp_nexthop.c @@ -474,7 +474,7 @@ int bgp_nexthop_self(struct bgp *bgp, afi_t afi, uint8_t type, uint8_t sub_type, struct attr *attr, struct bgp_node *rn) { uint8_t new_afi = afi == AFI_IP ? AF_INET : AF_INET6; - struct bgp_addr tmp_addr = {0}, *addr = NULL; + struct bgp_addr tmp_addr = {{0}}, *addr = NULL; struct tip_addr tmp_tip, *tip = NULL; bool is_bgp_static_route =