From ed216282b6ce3da844e254506d390807fcc0ad36 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 24 Jan 2018 09:16:39 -0500 Subject: [PATCH] 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, -- 2.39.5