Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: move common NHT update decoding bits into lib/ #14834

Merged
48 changes: 20 additions & 28 deletions bgpd/bgp_nht.c
Original file line number Diff line number Diff line change
Expand Up @@ -897,52 +897,44 @@ void bgp_nht_interface_events(struct peer *peer)
bnc->ifindex_ipv6_ll, NULL);
}

void bgp_parse_nexthop_update(int command, vrf_id_t vrf_id)
void bgp_nexthop_update(struct vrf *vrf, struct prefix *match,
struct zapi_route *nhr)
{
struct bgp_nexthop_cache_head *tree = NULL;
struct bgp_nexthop_cache *bnc_nhc, *bnc_import;
struct bgp *bgp;
struct prefix match;
struct zapi_route nhr;
afi_t afi;

bgp = bgp_lookup_by_vrf_id(vrf_id);
if (!bgp) {
flog_err(
EC_BGP_NH_UPD,
"parse nexthop update: instance not found for vrf_id %u",
vrf_id);
if (!vrf->info) {
flog_err(EC_BGP_NH_UPD,
"parse nexthop update: instance not found for vrf_id %u",
vrf->vrf_id);
return;
}

if (!zapi_nexthop_update_decode(zclient->ibuf, &match, &nhr)) {
zlog_err("%s[%s]: Failure to decode nexthop update", __func__,
bgp->name_pretty);
return;
}

afi = family2afi(match.family);
bgp = (struct bgp *)vrf->info;
afi = family2afi(match->family);
tree = &bgp->nexthop_cache_table[afi];

bnc_nhc = bnc_find(tree, &match, nhr.srte_color, 0);
bnc_nhc = bnc_find(tree, match, nhr->srte_color, 0);
if (!bnc_nhc) {
if (BGP_DEBUG(nht, NHT))
zlog_debug(
"parse nexthop update %pFX(%u)(%s): bnc info not found for nexthop cache",
&nhr.prefix, nhr.srte_color, bgp->name_pretty);
zlog_debug("parse nexthop update %pFX(%u)(%s): bnc info not found for nexthop cache",
&nhr->prefix, nhr->srte_color,
bgp->name_pretty);
} else
bgp_process_nexthop_update(bnc_nhc, &nhr, false);
bgp_process_nexthop_update(bnc_nhc, nhr, false);

tree = &bgp->import_check_table[afi];

bnc_import = bnc_find(tree, &match, nhr.srte_color, 0);
bnc_import = bnc_find(tree, match, nhr->srte_color, 0);
if (!bnc_import) {
if (BGP_DEBUG(nht, NHT))
zlog_debug(
"parse nexthop update %pFX(%u)(%s): bnc info not found for import check",
&nhr.prefix, nhr.srte_color, bgp->name_pretty);
zlog_debug("parse nexthop update %pFX(%u)(%s): bnc info not found for import check",
&nhr->prefix, nhr->srte_color,
bgp->name_pretty);
} else
bgp_process_nexthop_update(bnc_import, &nhr, true);
bgp_process_nexthop_update(bnc_import, nhr, true);

/*
* HACK: if any BGP route is dependant on an SR-policy that doesn't
Expand All @@ -955,7 +947,7 @@ void bgp_parse_nexthop_update(int command, vrf_id_t vrf_id)
* which should provide a better infrastructure to solve this issue in
* a more efficient and elegant way.
*/
if (nhr.srte_color == 0 && bnc_nhc) {
if (nhr->srte_color == 0 && bnc_nhc) {
struct bgp_nexthop_cache *bnc_iter;

frr_each (bgp_nexthop_cache, &bgp->nexthop_cache_table[afi],
Expand All @@ -965,7 +957,7 @@ void bgp_parse_nexthop_update(int command, vrf_id_t vrf_id)
CHECK_FLAG(bnc_iter->flags, BGP_NEXTHOP_VALID))
continue;

bgp_process_nexthop_update(bnc_iter, &nhr, false);
bgp_process_nexthop_update(bnc_iter, nhr, false);
}
}
}
Expand Down
5 changes: 3 additions & 2 deletions bgpd/bgp_nht.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@
#define _BGP_NHT_H

/**
* bgp_parse_nexthop_update() - parse a nexthop update message from Zebra.
* bgp_nexthop_update() - process a nexthop update message from Zebra.
*/
extern void bgp_parse_nexthop_update(int command, vrf_id_t vrf_id);
extern void bgp_nexthop_update(struct vrf *vrf, struct prefix *match,
struct zapi_route *nhr);

/**
* bgp_find_or_add_nexthop() - lookup the nexthop cache table for the bnc
Expand Down
9 changes: 1 addition & 8 deletions bgpd/bgp_zebra.c
Original file line number Diff line number Diff line change
Expand Up @@ -100,13 +100,6 @@ static int bgp_router_id_update(ZAPI_CALLBACK_ARGS)
return 0;
}

/* Nexthop update message from zebra. */
static int bgp_read_nexthop_update(ZAPI_CALLBACK_ARGS)
{
bgp_parse_nexthop_update(cmd, vrf_id);
return 0;
}

/* Set or clear interface on which unnumbered neighbor is configured. This
* would in turn cause BGP to initiate or turn off IPv6 RAs on this
* interface.
Expand Down Expand Up @@ -3332,7 +3325,6 @@ static zclient_handler *const bgp_handlers[] = {
[ZEBRA_INTERFACE_NBR_ADDRESS_DELETE] = bgp_interface_nbr_address_delete,
[ZEBRA_REDISTRIBUTE_ROUTE_ADD] = zebra_read_route,
[ZEBRA_REDISTRIBUTE_ROUTE_DEL] = zebra_read_route,
[ZEBRA_NEXTHOP_UPDATE] = bgp_read_nexthop_update,
[ZEBRA_FEC_UPDATE] = bgp_read_fec_update,
[ZEBRA_LOCAL_ES_ADD] = bgp_zebra_process_local_es_add,
[ZEBRA_LOCAL_ES_DEL] = bgp_zebra_process_local_es_del,
Expand Down Expand Up @@ -3454,6 +3446,7 @@ void bgp_zebra_init(struct event_loop *master, unsigned short instance)
zclient_init(zclient, ZEBRA_ROUTE_BGP, 0, &bgpd_privs);
zclient->zebra_connected = bgp_zebra_connected;
zclient->zebra_capabilities = bgp_zebra_capabilities;
zclient->nexthop_update = bgp_nexthop_update;
zclient->instance = instance;

/* Initialize special zclient for synchronous message exchanges. */
Expand Down
29 changes: 27 additions & 2 deletions lib/zclient.c
Original file line number Diff line number Diff line change
Expand Up @@ -2269,8 +2269,8 @@ const char *zapi_nexthop2str(const struct zapi_nexthop *znh, char *buf,
/*
* Decode the nexthop-tracking update message
*/
bool zapi_nexthop_update_decode(struct stream *s, struct prefix *match,
struct zapi_route *nhr)
static bool zapi_nexthop_update_decode(struct stream *s, struct prefix *match,
struct zapi_route *nhr)
{
uint32_t i;

Expand Down Expand Up @@ -4298,6 +4298,28 @@ int zapi_client_close_notify_decode(struct stream *s,
return -1;
}

static int zclient_nexthop_update(ZAPI_CALLBACK_ARGS)
{
struct vrf *vrf = vrf_lookup_by_id(vrf_id);
struct prefix match;
struct zapi_route route;

if (!vrf) {
zlog_warn("nexthop update for unknown VRF ID %u", vrf_id);
return 0;
}

if (!zapi_nexthop_update_decode(zclient->ibuf, &match, &route)) {
zlog_err("failed to decode nexthop update");
return -1;
}

if (zclient->nexthop_update)
zclient->nexthop_update(vrf, &match, &route);

return 0;
}

static zclient_handler *const lib_handlers[] = {
/* fundamentals */
[ZEBRA_CAPABILITIES] = zclient_capability_decode,
Expand All @@ -4311,6 +4333,9 @@ static zclient_handler *const lib_handlers[] = {
[ZEBRA_INTERFACE_UP] = zclient_interface_up,
[ZEBRA_INTERFACE_DOWN] = zclient_interface_down,

/* NHT pre-decode */
[ZEBRA_NEXTHOP_UPDATE] = zclient_nexthop_update,

/* BFD */
[ZEBRA_BFD_DEST_REPLAY] = zclient_bfd_session_replay,
[ZEBRA_INTERFACE_BFD_DEST_UPDATE] = zclient_bfd_session_update,
Expand Down
27 changes: 15 additions & 12 deletions lib/zclient.h
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,8 @@ struct zapi_cap {
typedef int (zclient_handler)(ZAPI_CALLBACK_ARGS);
/* clang-format on */

struct zapi_route;

/* Structure for the zebra client. */
struct zclient {
/* The thread master we schedule ourselves on */
Expand Down Expand Up @@ -348,6 +350,19 @@ struct zclient {
void (*zebra_connected)(struct zclient *);
void (*zebra_capabilities)(struct zclient_capabilities *cap);

/*
* match -> is the prefix that the calling daemon asked to be matched
* against.
* nhr->prefix -> is the actual prefix that was matched against in the
* rib itself.
*
* This distinction is made because a LPM can be made if there is a
* covering route. This way the upper level protocol can make a
* decision point about whether or not it wants to use the match or not.
*/
void (*nexthop_update)(struct vrf *vrf, struct prefix *match,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can 'match' and 'nhr' be 'const'?

Copy link
Contributor Author

@eqvinox eqvinox Nov 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that and it had a trail of fallout (from other functions taking non-const args) behind it, such that I didn't want to shove into this PR to keep it clean-ish 😞

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'nexthop_update' seems a bit ambiguous - now that we have nexthop-groups etc. could it be 'nht_update' or 'rnh_update' ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to rename it, I just "inherited" the name from zapi_nexthop_update_decode. Donald and I also have renaming the entirety of NHT on the TODO list (because the name is kinda misleading), maybe it doesn't matter at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I don't think rnh is a good choice of name, this isn't about recursive resolution; nht is off in general since this isn't a mechanism for specifically tracking nexthops, it tracks prefixes in general… cf. discussion in #development)

struct zapi_route *nhr);

int (*handle_error)(enum zebra_error_types error);

/*
Expand Down Expand Up @@ -1123,18 +1138,6 @@ int zapi_nexthop_from_nexthop(struct zapi_nexthop *znh,
const struct nexthop *nh);
int zapi_backup_nexthop_from_nexthop(struct zapi_nexthop *znh,
const struct nexthop *nh);
/*
* match -> is the prefix that the calling daemon asked to be matched
* against.
* nhr->prefix -> is the actual prefix that was matched against in the
* rib itself.
*
* This distinction is made because a LPM can be made if there is a
* covering route. This way the upper level protocol can make a decision
* point about whether or not it wants to use the match or not.
*/
extern bool zapi_nexthop_update_decode(struct stream *s, struct prefix *match,
struct zapi_route *nhr);
const char *zapi_nexthop2str(const struct zapi_nexthop *znh, char *buf,
int bufsize);

Expand Down
28 changes: 10 additions & 18 deletions ospf6d/ospf6_zebra.c
Original file line number Diff line number Diff line change
Expand Up @@ -147,30 +147,22 @@ void ospf6_zebra_import_default_route(struct ospf6 *ospf6, bool unreg)
__func__);
}

static int ospf6_zebra_import_check_update(ZAPI_CALLBACK_ARGS)
static void ospf6_zebra_import_check_update(struct vrf *vrf,
struct prefix *matched,
struct zapi_route *nhr)
{
struct ospf6 *ospf6;
struct zapi_route nhr;
struct prefix matched;

ospf6 = ospf6_lookup_by_vrf_id(vrf_id);
ospf6 = (struct ospf6 *)vrf->info;
if (ospf6 == NULL || !IS_OSPF6_ASBR(ospf6))
return 0;

if (!zapi_nexthop_update_decode(zclient->ibuf, &matched, &nhr)) {
zlog_err("%s[%u]: Failure to decode route", __func__,
ospf6->vrf_id);
return -1;
}
return;

if (matched.family != AF_INET6 || matched.prefixlen != 0 ||
nhr.type == ZEBRA_ROUTE_OSPF6)
return 0;
if (matched->family != AF_INET6 || matched->prefixlen != 0 ||
nhr->type == ZEBRA_ROUTE_OSPF6)
return;

ospf6->nssa_default_import_check.status = !!nhr.nexthop_num;
ospf6->nssa_default_import_check.status = !!nhr->nexthop_num;
ospf6_abr_nssa_type_7_defaults(ospf6);

return 0;
}

static int ospf6_zebra_if_address_update_add(ZAPI_CALLBACK_ARGS)
Expand Down Expand Up @@ -763,7 +755,6 @@ static zclient_handler *const ospf6_handlers[] = {
[ZEBRA_INTERFACE_ADDRESS_DELETE] = ospf6_zebra_if_address_update_delete,
[ZEBRA_REDISTRIBUTE_ROUTE_ADD] = ospf6_zebra_read_route,
[ZEBRA_REDISTRIBUTE_ROUTE_DEL] = ospf6_zebra_read_route,
[ZEBRA_NEXTHOP_UPDATE] = ospf6_zebra_import_check_update,
};

void ospf6_zebra_init(struct event_loop *master)
Expand All @@ -773,6 +764,7 @@ void ospf6_zebra_init(struct event_loop *master)
array_size(ospf6_handlers));
zclient_init(zclient, ZEBRA_ROUTE_OSPF6, 0, &ospf6d_privs);
zclient->zebra_connected = ospf6_zebra_connected;
zclient->nexthop_update = ospf6_zebra_import_check_update;

/* Install command element for zebra node. */
install_element(VIEW_NODE, &show_ospf6_zebra_cmd);
Expand Down
28 changes: 9 additions & 19 deletions ospfd/ospf_zebra.c
Original file line number Diff line number Diff line change
Expand Up @@ -1487,30 +1487,20 @@ void ospf_zebra_import_default_route(struct ospf *ospf, bool unreg)
__func__);
}

static int ospf_zebra_import_check_update(ZAPI_CALLBACK_ARGS)
static void ospf_zebra_import_check_update(struct vrf *vrf, struct prefix *match,
struct zapi_route *nhr)
{
struct ospf *ospf;
struct zapi_route nhr;
struct prefix matched;
struct ospf *ospf = vrf->info;

ospf = ospf_lookup_by_vrf_id(vrf_id);
if (ospf == NULL || !IS_OSPF_ASBR(ospf))
return 0;

if (!zapi_nexthop_update_decode(zclient->ibuf, &matched, &nhr)) {
zlog_err("%s[%u]: Failure to decode route", __func__,
ospf->vrf_id);
return -1;
}
return;

if (matched.family != AF_INET || matched.prefixlen != 0 ||
nhr.type == ZEBRA_ROUTE_OSPF)
return 0;
if (match->family != AF_INET || match->prefixlen != 0 ||
nhr->type == ZEBRA_ROUTE_OSPF)
return;

ospf->nssa_default_import_check.status = !!nhr.nexthop_num;
ospf->nssa_default_import_check.status = !!nhr->nexthop_num;
ospf_abr_nssa_type7_defaults(ospf);

return 0;
}

int ospf_distribute_list_out_set(struct ospf *ospf, int type, const char *name)
Expand Down Expand Up @@ -2183,7 +2173,6 @@ static zclient_handler *const ospf_handlers[] = {

[ZEBRA_REDISTRIBUTE_ROUTE_ADD] = ospf_zebra_read_route,
[ZEBRA_REDISTRIBUTE_ROUTE_DEL] = ospf_zebra_read_route,
[ZEBRA_NEXTHOP_UPDATE] = ospf_zebra_import_check_update,

[ZEBRA_OPAQUE_MESSAGE] = ospf_opaque_msg_handler,

Expand All @@ -2197,6 +2186,7 @@ void ospf_zebra_init(struct event_loop *master, unsigned short instance)
array_size(ospf_handlers));
zclient_init(zclient, ZEBRA_ROUTE_OSPF, instance, &ospfd_privs);
zclient->zebra_connected = ospf_zebra_connected;
zclient->nexthop_update = ospf_zebra_import_check_update;

/* Initialize special zclient for synchronous message exchanges. */
struct zclient_options options = zclient_options_default;
Expand Down
Loading
Loading