fix stale pointers in northbound nodes #8095
Conversation
There was a problem hiding this comment.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/d802213a73edf58da38cea6360ed2509/raw/c9a1cb59763a172bc71762dab623869732707b05/cr_8095_1613473272.diff | git apply
diff --git a/lib/northbound.c b/lib/northbound.c
index f72f41936..d5f1ac40a 100644
--- a/lib/northbound.c
+++ b/lib/northbound.c
@@ -568,17 +568,19 @@ int nb_candidate_edit(struct nb_config *candidate,
/*
* create dependency
*
- * dnode returned by the lyd_new_path may be from different
- * schema, so we need to update the nb_node
+ * dnode returned by the lyd_new_path may be from
+ * different schema, so we need to update the nb_node
*/
nb_node = dnode->schema->priv;
if (nb_node->dep_cbs.get_dependency_xpath) {
- nb_node->dep_cbs.get_dependency_xpath(dnode, dep_xpath);
+ nb_node->dep_cbs.get_dependency_xpath(
+ dnode, dep_xpath);
ly_errno = 0;
- dep_dnode = lyd_new_path(candidate->dnode, ly_native_ctx,
- dep_xpath, NULL, 0,
- LYD_PATH_OPT_UPDATE);
+ dep_dnode = lyd_new_path(candidate->dnode,
+ ly_native_ctx,
+ dep_xpath, NULL, 0,
+ LYD_PATH_OPT_UPDATE);
if (!dep_dnode && ly_errno) {
flog_warn(EC_LIB_LIBYANG,
"%s: lyd_new_path() failed",
diff --git a/lib/vrf.c b/lib/vrf.c
index 63feb9421..0a91f4bc8 100644
--- a/lib/vrf.c
+++ b/lib/vrf.c
@@ -686,8 +686,8 @@ int vrf_handler_create(struct vty *vty, const char *vrfname,
}
if (vty) {
- snprintf(xpath_list, sizeof(xpath_list),
- FRR_VRF_KEY_XPATH, vrfname);
+ snprintf(xpath_list, sizeof(xpath_list), FRR_VRF_KEY_XPATH,
+ vrfname);
nb_cli_enqueue_change(vty, xpath_list, NB_OP_CREATE, NULL);
ret = nb_cli_apply_changes(vty, xpath_list);
@@ -821,8 +821,7 @@ DEFUN_YANG (no_vrf,
return CMD_WARNING_CONFIG_FAILED;
}
- snprintf(xpath_list, sizeof(xpath_list), FRR_VRF_KEY_XPATH,
- vrfname);
+ snprintf(xpath_list, sizeof(xpath_list), FRR_VRF_KEY_XPATH, vrfname);
nb_cli_enqueue_change(vty, xpath_list, NB_OP_DESTROY, NULL);
return nb_cli_apply_changes(vty, xpath_list);
diff --git a/pimd/pim_nb_config.c b/pimd/pim_nb_config.c
index 39344c4b3..ceb20a69d 100644
--- a/pimd/pim_nb_config.c
+++ b/pimd/pim_nb_config.c
@@ -3051,17 +3051,19 @@ int lib_interface_igmp_address_family_static_group_destroy(
return NB_OK;
}
-static void vrf_to_control_plane_protocol(const struct lyd_node *dnode, char *xpath)
+static void vrf_to_control_plane_protocol(const struct lyd_node *dnode,
+ char *xpath)
{
const char *vrf;
vrf = yang_dnode_get_string(dnode, "./name");
- snprintf(xpath, XPATH_MAXLEN, FRR_ROUTING_KEY_XPATH,
- "frr-pim:pimd", "pim", vrf);
+ snprintf(xpath, XPATH_MAXLEN, FRR_ROUTING_KEY_XPATH, "frr-pim:pimd",
+ "pim", vrf);
}
-static void control_plane_protocol_to_vrf(const struct lyd_node *dnode, char *xpath)
+static void control_plane_protocol_to_vrf(const struct lyd_node *dnode,
+ char *xpath)
{
const char *vrf;
diff --git a/staticd/static_nb_config.c b/staticd/static_nb_config.c
index 04dea95ef..2cbde803d 100644
--- a/staticd/static_nb_config.c
+++ b/staticd/static_nb_config.c
@@ -1227,7 +1227,8 @@ int routing_control_plane_protocols_control_plane_protocol_staticd_route_list_sr
return NB_OK;
}
-static void vrf_to_control_plane_protocol(const struct lyd_node *dnode, char *xpath)
+static void vrf_to_control_plane_protocol(const struct lyd_node *dnode,
+ char *xpath)
{
const char *vrf;
@@ -1237,7 +1238,8 @@ static void vrf_to_control_plane_protocol(const struct lyd_node *dnode, char *xp
"frr-staticd:staticd", "staticd", vrf);
}
-static void control_plane_protocol_to_vrf(const struct lyd_node *dnode, char *xpath)
+static void control_plane_protocol_to_vrf(const struct lyd_node *dnode,
+ char *xpath)
{
const char *vrf;
If you are a new contributor to FRR, please see our contributing guidelines.
After making changes, you do not need to create a new PR. You should perform an amend or interactive rebase followed by a force push.
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17172/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warningsWarnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: CLANG Static Analyzer Summary
Fixed warnings:
New warnings:
Static Analysis warning summary compared to base:
5 Static Analyzer issues remaining.See details at |
idryzhov
force-pushed
the
fix-nb-stale-pointers
branch
from
c1ae678 to
7ab85ae
Compare
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17175/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warningsWarnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: CLANG Static Analyzer Summary
5 Static Analyzer issues remaining.See details at |
| @@ -87,12 +87,12 @@ int bgp_router_create(struct nb_cb_create_args *args) | |||
| case NB_EV_APPLY: | |||
| vrf_dnode = yang_dnode_get_parent(args->dnode, | |||
| "control-plane-protocol"); | |||
| vrf = nb_running_get_entry(vrf_dnode, NULL, true); | |||
| vrf_name = yang_dnode_get_string(vrf_dnode, "./vrf"); | |||
|
|
|||
There was a problem hiding this comment.
dont you have to register for bgp too ?
There was a problem hiding this comment.
No. This one was the only place where it was using the vrf. Now it just gets the name from control-plane-protocol dnode, it doesn't need the actual vrf to be created.
More than that, we don't want to delete the "router bgp" configuration when the vrf is deleted.
There was a problem hiding this comment.
your answer is ok for me. thanks.
ok, I realise that rpki plugin has been magically ignored from the nb config porting.
|
@idryzhov could you fix the 1 warning generated? |
idryzhov
force-pushed
the
fix-nb-stale-pointers
branch
from
7ab85ae to
91305bf
Compare
|
@donaldsharp done |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17204/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: CLANG Static Analyzer Summary
Fixed warnings:
New warnings:
Static Analysis warning summary compared to base:
5 Static Analyzer issues remaining.See details at |
| dep_xpath, NULL, 0, | ||
| LYD_PATH_OPT_UPDATE); | ||
| if (!dep_dnode && ly_errno) { | ||
| flog_warn(EC_LIB_LIBYANG, |
There was a problem hiding this comment.
nit : same error message for dnode and dep_dnode creation failure, It would be difficult to get the exact failure from logs.
| dnode, dep_xpath); | ||
|
|
||
| ly_errno = 0; | ||
| dep_dnode = lyd_new_path(candidate->dnode, |
There was a problem hiding this comment.
Consider one scenario
c t
ip route 1.1.1.1/32 ens33
In this configuration you will create a default dnode in frr-vrf yang table.
Now I have deleted the route
c t
no ip route 1.1.1.1/32 ens33
Now default dnode is not getting deleted from frr-vrf yang table.
There was a problem hiding this comment.
This issue is not directly related to this PR. This already happens for any other configuration that needs default VRF dnode. For example, when you configure vni in zebra.
I'm not sure that this is an issue at all. Nothing wrong with always having a configuration node for the default VRF as this VRF always exists.
Anyway, this should not be a blocker for this fix and can be discussed and fixed separately.
There was a problem hiding this comment.
ok, we can discuss and fixed separately.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
When the control plane protocol is created, the vrf structure is allocated, and its address is stored in the northbound node. The vrf structure may later be deleted by the user, which will lead to a stale pointer stored in this node. Instead of this, allow daemons that use the vrf pointer to register the dependency between the control plane protocol and vrf nodes. This will guarantee that the nodes will always be created and deleted together, and there won't be any stale pointers. Add such registration to staticd and pimd. Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
idryzhov
force-pushed
the
fix-nb-stale-pointers
branch
from
91305bf to
2ada626
Compare
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17250/ This is a comment from an automated CI system. CLANG Static Analyzer Summary
4 Static Analyzer issues remaining.See details at |
|
PR looks good to me. |
Overall, this PR introduces the ability to register dependency between NB nodes to fix the stale pointers issue.
For more explanation, please, check individual commits.