From 37f861f1dbeda410c52d3b5f9f685e32a177211c Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Thu, 23 Nov 2023 20:21:31 -0300 Subject: [PATCH 1/2] ospfd: improve memory cleanup during shutdown * On ospf_terminate(), proceed to clear the ospfd global variables even when no OSPF instance is configured * Remove double call to route_map_finish() * Call ospf_opaque_term() to clear the opaque LSA infrastructure * Clear the `OspfRI.area_info` and `om->ospf` global lists. Signed-off-by: Renato Westphal --- ospfd/ospf_ri.c | 1 + ospfd/ospfd.c | 11 ++++------- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/ospfd/ospf_ri.c b/ospfd/ospf_ri.c index 725443f490..c6aaf3f5a0 100644 --- a/ospfd/ospf_ri.c +++ b/ospfd/ospf_ri.c @@ -153,6 +153,7 @@ static int ospf_router_info_unregister(void) void ospf_router_info_term(void) { + list_delete(&OspfRI.area_info); list_delete(&OspfRI.pce_info.pce_domain); list_delete(&OspfRI.pce_info.pce_neighbor); diff --git a/ospfd/ospfd.c b/ospfd/ospfd.c index d5cf9564ff..b3f4cad5c1 100644 --- a/ospfd/ospfd.c +++ b/ospfd/ospfd.c @@ -658,10 +658,6 @@ void ospf_terminate(void) SET_FLAG(om->options, OSPF_MASTER_SHUTDOWN); - /* Skip some steps if OSPF not actually running */ - if (listcount(om->ospf) == 0) - goto done; - for (ALL_LIST_ELEMENTS(om->ospf, node, nnode, ospf)) ospf_finish(ospf); @@ -679,9 +675,11 @@ void ospf_terminate(void) /* Cleanup vrf info */ ospf_vrf_terminate(); - route_map_finish(); - keychain_terminate(); + + ospf_opaque_term(); + list_delete(&om->ospf); + /* Deliberately go back up, hopefully to thread scheduler, as * One or more ospf_finish()'s may have deferred shutdown to a timer * thread @@ -691,7 +689,6 @@ void ospf_terminate(void) zclient_stop(zclient_sync); zclient_free(zclient_sync); -done: frr_fini(); } From 3e2f053b12371c71d8f7c21a4df68d21d4682637 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Thu, 23 Nov 2023 20:23:52 -0300 Subject: [PATCH 2/2] ospfd: fix deferred shutdown handling The ospfd cleanup code is relatively complicated given the need to appropriately handle the "max-metric router-lsa on-shutdown (5-100)" command. When that command is configured and an OSPF instance is unconfigured, the removal of the instance should be deferred to allow other routers sufficient time to find alternate paths before the local Router-LSAs are flushed. When ospfd is killed, however, deferred shutdown shouldn't take place and all instances should be cleared immediately. This commit fixes a problem where ospf_deferred_shutdown_finish() was prematurely exiting the daemon when no instances were left, inadvertently preventing ospf_terminate() from clearing the ospfd global variables. Additionally, the commit includes code refactoring to enhance readability and maintainability. Fixes #14855. Signed-off-by: Renato Westphal --- ospfd/ospfd.c | 56 +++++++++++---------------------------------------- 1 file changed, 12 insertions(+), 44 deletions(-) diff --git a/ospfd/ospfd.c b/ospfd/ospfd.c index b3f4cad5c1..4c4666db52 100644 --- a/ospfd/ospfd.c +++ b/ospfd/ospfd.c @@ -575,40 +575,12 @@ static struct ospf *ospf_lookup_by_name(const char *vrf_name) return NULL; } -/* Handle the second half of deferred shutdown. This is called either - * from the deferred-shutdown timer thread, or directly through - * ospf_deferred_shutdown_check. - * - * Function is to cleanup G-R state, if required then call ospf_finish_final - * to complete shutdown of this ospf instance. Possibly exit if the - * whole process is being shutdown and this was the last OSPF instance. - */ -static void ospf_deferred_shutdown_finish(struct ospf *ospf) -{ - ospf->stub_router_shutdown_time = OSPF_STUB_ROUTER_UNCONFIGURED; - EVENT_OFF(ospf->t_deferred_shutdown); - - ospf_finish_final(ospf); - - /* *ospf is now invalid */ - - /* ospfd being shut-down? If so, was this the last ospf instance? */ - if (CHECK_FLAG(om->options, OSPF_MASTER_SHUTDOWN) - && (listcount(om->ospf) == 0)) { - route_map_finish(); - frr_fini(); - exit(0); - } - - return; -} - -/* Timer thread for G-R */ +/* Timer thread for deferred shutdown */ static void ospf_deferred_shutdown_timer(struct event *t) { struct ospf *ospf = EVENT_ARG(t); - ospf_deferred_shutdown_finish(ospf); + ospf_finish_final(ospf); } /* Check whether deferred-shutdown must be scheduled, otherwise call @@ -635,15 +607,12 @@ static void ospf_deferred_shutdown_check(struct ospf *ospf) ospf_router_lsa_update_area(area); } timeout = ospf->stub_router_shutdown_time; + OSPF_TIMER_ON(ospf->t_deferred_shutdown, + ospf_deferred_shutdown_timer, timeout); } else { /* No timer needed */ - ospf_deferred_shutdown_finish(ospf); - return; + ospf_finish_final(ospf); } - - OSPF_TIMER_ON(ospf->t_deferred_shutdown, ospf_deferred_shutdown_timer, - timeout); - return; } /* Shut down the entire process */ @@ -694,14 +663,12 @@ void ospf_terminate(void) void ospf_finish(struct ospf *ospf) { - /* let deferred shutdown decide */ - ospf_deferred_shutdown_check(ospf); - - /* if ospf_deferred_shutdown returns, then ospf_finish_final is - * deferred to expiry of G-S timer thread. Return back up, hopefully - * to thread scheduler. - */ - return; + if (CHECK_FLAG(om->options, OSPF_MASTER_SHUTDOWN)) + ospf_finish_final(ospf); + else { + /* let deferred shutdown decide */ + ospf_deferred_shutdown_check(ospf); + } } /* Final cleanup of ospf instance */ @@ -901,6 +868,7 @@ static void ospf_finish_final(struct ospf *ospf) EVENT_OFF(ospf->t_ase_calc); EVENT_OFF(ospf->t_maxage); EVENT_OFF(ospf->t_maxage_walker); + EVENT_OFF(ospf->t_deferred_shutdown); EVENT_OFF(ospf->t_abr_task); EVENT_OFF(ospf->t_abr_fr); EVENT_OFF(ospf->t_asbr_check);