From 7d974ba3b72ce8c4e46201a3c8b60e1680406883 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 24 Jan 2018 08:22:57 -0500 Subject: [PATCH 1/2] zebra: Modify southbound interface to pass `struct route_node` The route_node that we are working on is going to be interesting to the kernel_route_rib_pass_fail. So I am setting up the code to allow me to pass it. This will be done in a subsuquent commit. Signed-off-by: Donald Sharp --- zebra/rt.h | 8 +++++--- zebra/rt_netlink.c | 9 +++++---- zebra/rt_socket.c | 9 +++++---- zebra/zebra_rib.c | 7 ++++--- 4 files changed, 19 insertions(+), 14 deletions(-) diff --git a/zebra/rt.h b/zebra/rt.h index bb4ff5bee8..54d45b889a 100644 --- a/zebra/rt.h +++ b/zebra/rt.h @@ -60,15 +60,17 @@ enum southbound_results { * semantics so we will end up with a delete than * a re-add. */ -extern void kernel_route_rib(struct prefix *p, struct prefix *src_p, - struct route_entry *old, struct route_entry *new); +extern void kernel_route_rib(struct route_node *rn, struct prefix *p, + struct prefix *src_p, struct route_entry *old, + struct route_entry *new); /* * So route install/failure may not be immediately known * so let's separate it out and allow the result to * be passed back up. */ -extern void kernel_route_rib_pass_fail(struct prefix *p, +extern void kernel_route_rib_pass_fail(struct route_node *rn, + struct prefix *p, struct route_entry *re, enum southbound_results res); diff --git a/zebra/rt_netlink.c b/zebra/rt_netlink.c index 0221162a30..20cc292e11 100644 --- a/zebra/rt_netlink.c +++ b/zebra/rt_netlink.c @@ -1642,8 +1642,9 @@ int kernel_get_ipmr_sg_stats(struct zebra_vrf *zvrf, void *in) return suc; } -void kernel_route_rib(struct prefix *p, struct prefix *src_p, - struct route_entry *old, struct route_entry *new) +void kernel_route_rib(struct route_node *rn, struct prefix *p, + struct prefix *src_p, struct route_entry *old, + struct route_entry *new) { int ret = 0; @@ -1672,7 +1673,7 @@ void kernel_route_rib(struct prefix *p, struct prefix *src_p, ret = netlink_route_multipath(RTM_NEWROUTE, p, src_p, new, 0); } - kernel_route_rib_pass_fail(p, new, + kernel_route_rib_pass_fail(rn, p, new, (!ret) ? SOUTHBOUND_INSTALL_SUCCESS : SOUTHBOUND_INSTALL_FAILURE); @@ -1682,7 +1683,7 @@ void kernel_route_rib(struct prefix *p, struct prefix *src_p, if (old) { ret = netlink_route_multipath(RTM_DELROUTE, p, src_p, old, 0); - kernel_route_rib_pass_fail(p, old, + kernel_route_rib_pass_fail(rn, p, old, (!ret) ? SOUTHBOUND_DELETE_SUCCESS : SOUTHBOUND_DELETE_FAILURE); diff --git a/zebra/rt_socket.c b/zebra/rt_socket.c index 09fdf0b2d3..6d4af1203c 100644 --- a/zebra/rt_socket.c +++ b/zebra/rt_socket.c @@ -387,8 +387,9 @@ static int kernel_rtm(int cmd, struct prefix *p, struct route_entry *re) return 0; } -void kernel_route_rib(struct prefix *p, struct prefix *src_p, - struct route_entry *old, struct route_entry *new) +void kernel_route_rib(struct route_node *rn, struct prefix *p, + struct prefix *src_p, struct route_entry *old, + struct route_entry *new) { int route = 0; @@ -410,12 +411,12 @@ void kernel_route_rib(struct prefix *p, struct prefix *src_p, zlog_err("Can't lower privileges"); if (new) { - kernel_route_rib_pass_fail(p, new, + kernel_route_rib_pass_fail(rn, p, new, (!route) ? SOUTHBOUND_INSTALL_SUCCESS : SOUTHBOUND_INSTALL_FAILURE); } else { - kernel_route_rib_pass_fail(p, old, + kernel_route_rib_pass_fail(rn, p, old, (!route) ? SOUTHBOUND_DELETE_SUCCESS : SOUTHBOUND_DELETE_FAILURE); diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c index 78077a4b3d..83eff7bdcf 100644 --- a/zebra/zebra_rib.c +++ b/zebra/zebra_rib.c @@ -998,7 +998,8 @@ int zebra_rib_labeled_unicast(struct route_entry *re) return 1; } -void kernel_route_rib_pass_fail(struct prefix *p, struct route_entry *re, +void kernel_route_rib_pass_fail(struct route_node *rn, struct prefix *p, + struct route_entry *re, enum southbound_results res) { struct nexthop *nexthop; @@ -1083,7 +1084,7 @@ void rib_install_kernel(struct route_node *rn, struct route_entry *re, * the kernel. */ hook_call(rib_update, rn, "installing in kernel"); - kernel_route_rib(p, src_p, old, re); + kernel_route_rib(rn, p, src_p, old, re); zvrf->installs++; return; @@ -1110,7 +1111,7 @@ void rib_uninstall_kernel(struct route_node *rn, struct route_entry *re) * the kernel. */ hook_call(rib_update, rn, "uninstalling from kernel"); - kernel_route_rib(p, src_p, re, NULL); + kernel_route_rib(rn, p, src_p, re, NULL); zvrf->removals++; return; From ed216282b6ce3da844e254506d390807fcc0ad36 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 24 Jan 2018 09:16:39 -0500 Subject: [PATCH 2/2] zebra: Move selected_fib assignment The dest->selected_fib assignment needs to happen after the install and should be controlled by the southbound api return of success or failure. Signed-off-by: Donald Sharp --- zebra/zebra_rib.c | 78 +++++++++++++++++++++++++++++++++++++---------- 1 file changed, 62 insertions(+), 16 deletions(-) diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c index 83eff7bdcf..c200e2dbb3 100644 --- a/zebra/zebra_rib.c +++ b/zebra/zebra_rib.c @@ -1004,9 +1004,13 @@ void kernel_route_rib_pass_fail(struct route_node *rn, struct prefix *p, { struct nexthop *nexthop; char buf[PREFIX_STRLEN]; + rib_dest_t *dest; + + dest = rib_dest_from_rnode(rn); switch (res) { case SOUTHBOUND_INSTALL_SUCCESS: + dest->selected_fib = re; for (ALL_NEXTHOPS(re->nexthop, nexthop)) { if (CHECK_FLAG(nexthop->flags, NEXTHOP_FLAG_RECURSIVE)) continue; @@ -1020,16 +1024,37 @@ void kernel_route_rib_pass_fail(struct route_node *rn, struct prefix *p, p, ZAPI_ROUTE_INSTALLED); break; case SOUTHBOUND_INSTALL_FAILURE: + /* + * I am not sure this is the right thing to do here + * but the code always set selected_fib before + * this assignment was moved here. + */ + dest->selected_fib = re; + zsend_route_notify_owner(re->type, re->instance, re->vrf_id, p, ZAPI_ROUTE_FAIL_INSTALL); zlog_warn("%u:%s: Route install failed", re->vrf_id, prefix2str(p, buf, sizeof(buf))); break; case SOUTHBOUND_DELETE_SUCCESS: + /* + * The case where selected_fib is not re is + * when we have received a system route + * that is overriding our installed route + * as such we should leave the selected_fib + * pointer alone + */ + if (dest->selected_fib == re) + dest->selected_fib = NULL; for (ALL_NEXTHOPS(re->nexthop, nexthop)) UNSET_FLAG(nexthop->flags, NEXTHOP_FLAG_FIB); break; case SOUTHBOUND_DELETE_FAILURE: + /* + * Should we set this to NULL if the + * delete fails? + */ + dest->selected_fib = NULL; zlog_warn("%u:%s: Route Deletion failure", re->vrf_id, prefix2str(p, buf, sizeof(buf))); break; @@ -1133,8 +1158,6 @@ static void rib_uninstall(struct route_node *rn, struct route_entry *re) /* If labeled-unicast route, uninstall transit LSP. */ if (zebra_rib_labeled_unicast(re)) zebra_mpls_lsp_uninstall(info->zvrf, rn, re); - - dest->selected_fib = NULL; } if (CHECK_FLAG(re->flags, ZEBRA_FLAG_SELECTED)) { @@ -1219,7 +1242,6 @@ static void rib_process_add_fib(struct zebra_vrf *zvrf, struct route_node *rn, return; } - dest->selected_fib = new; if (IS_ZEBRA_DEBUG_RIB) { char buf[SRCDEST2STR_BUFFER]; srcdest_rnode2str(rn, buf, sizeof(buf)); @@ -1233,6 +1255,8 @@ static void rib_process_add_fib(struct zebra_vrf *zvrf, struct route_node *rn, if (!RIB_SYSTEM_ROUTE(new)) rib_install_kernel(rn, new, NULL); + else + dest->selected_fib = new; UNSET_FLAG(new->status, ROUTE_ENTRY_CHANGED); } @@ -1257,8 +1281,17 @@ static void rib_process_del_fib(struct zebra_vrf *zvrf, struct route_node *rn, if (!RIB_SYSTEM_ROUTE(old)) rib_uninstall_kernel(rn, old); - - dest->selected_fib = NULL; + else { + /* + * We are setting this to NULL here + * because that is what we traditionally + * have been doing. I am not positive + * that this is the right thing to do + * but let's leave the code alone + * for the RIB_SYSTEM_ROUTE case + */ + dest->selected_fib = NULL; + } /* Update nexthop for route, reset changed flag. */ nexthop_active_update(rn, old, 1); @@ -1272,7 +1305,6 @@ static void rib_process_update_fib(struct zebra_vrf *zvrf, { struct nexthop *nexthop = NULL; int nh_active = 0; - int installed = 1; rib_dest_t *dest = rib_dest_from_rnode(rn); /* @@ -1322,11 +1354,23 @@ static void rib_process_update_fib(struct zebra_vrf *zvrf, zebra_mpls_lsp_install(zvrf, rn, new); rib_install_kernel(rn, new, old); + } else { + /* + * We do not need to install the + * selected route because it + * is already isntalled by + * the system( ie not us ) + * so just mark it as winning + * we do need to ensure that + * if we uninstall a route + * from ourselves we don't + * over write this pointer + */ + dest->selected_fib = NULL; } - /* If install succeeded or system route, cleanup flags * for prior route. */ - if (installed && new != old) { + if (new != old) { if (RIB_SYSTEM_ROUTE(new)) { if (!RIB_SYSTEM_ROUTE(old)) rib_uninstall_kernel(rn, old); @@ -1337,10 +1381,6 @@ static void rib_process_update_fib(struct zebra_vrf *zvrf, NEXTHOP_FLAG_FIB); } } - - /* Update for redistribution. */ - if (installed) - dest->selected_fib = new; } /* @@ -1348,7 +1388,7 @@ static void rib_process_update_fib(struct zebra_vrf *zvrf, * failed, we * may need to uninstall and delete for redistribution. */ - if (!nh_active || !installed) { + if (!nh_active) { if (IS_ZEBRA_DEBUG_RIB) { char buf[SRCDEST2STR_BUFFER]; srcdest_rnode2str(rn, buf, sizeof(buf)); @@ -1375,7 +1415,8 @@ static void rib_process_update_fib(struct zebra_vrf *zvrf, if (!RIB_SYSTEM_ROUTE(old)) rib_uninstall_kernel(rn, old); - dest->selected_fib = NULL; + else + dest->selected_fib = NULL; } } else { /* @@ -1388,12 +1429,12 @@ static void rib_process_update_fib(struct zebra_vrf *zvrf, * to add routes. */ if (!RIB_SYSTEM_ROUTE(new)) { - int in_fib = 0; + bool in_fib = false; for (ALL_NEXTHOPS(new->nexthop, nexthop)) if (CHECK_FLAG(nexthop->flags, NEXTHOP_FLAG_FIB)) { - in_fib = 1; + in_fib = true; break; } if (!in_fib) @@ -2440,6 +2481,11 @@ void rib_delete(afi_t afi, safi_t safi, vrf_id_t vrf_id, int type, UNSET_FLAG(rtnh->flags, NEXTHOP_FLAG_FIB); + /* + * This is a non FRR route + * as such we should mark + * it as deleted + */ dest->selected_fib = NULL; } else { /* This means someone else, other than Zebra,