From a9460ae713a43285a972070d925ce38eaa5e363a Mon Sep 17 00:00:00 2001 From: Igor Ryzhov Date: Sun, 25 Feb 2024 23:12:14 +0200 Subject: [PATCH 1/2] lib: fix access-list entry update When an access-list entry is updated, current NB code calls notification hooks for each updated field. It means that when multiple fields of an entry are changed in a single commit, the hooks are run with an interim state of an access-list instead of a final one. To fix the issue, we should call the hooks once, after all fields of an entry are updated. Signed-off-by: Igor Ryzhov --- lib/filter.c | 3 +++ lib/filter.h | 1 + lib/filter_nb.c | 69 +++++++------------------------------------------ 3 files changed, 14 insertions(+), 59 deletions(-) diff --git a/lib/filter.c b/lib/filter.c index a0adff0e35dc..5a0790f8bfc2 100644 --- a/lib/filter.c +++ b/lib/filter.c @@ -410,7 +410,10 @@ void access_list_filter_add(struct access_list *access, filter->prev = access->tail; access->tail = filter; } +} +void access_list_filter_update(struct access_list *access) +{ /* Run hook function. */ if (access->master->add_hook) (*access->master->add_hook)(access); diff --git a/lib/filter.h b/lib/filter.h index bd9e22d38416..4fa482ba4ee7 100644 --- a/lib/filter.h +++ b/lib/filter.h @@ -128,6 +128,7 @@ struct filter *filter_new(void); void access_list_filter_add(struct access_list *access, struct filter *filter); void access_list_filter_delete(struct access_list *access, struct filter *filter); +void access_list_filter_update(struct access_list *access); int64_t filter_new_seq_get(struct access_list *access); extern const struct frr_yang_module_info frr_filter_info; diff --git a/lib/filter_nb.c b/lib/filter_nb.c index ec31f6d39558..91f80dcb5e86 100644 --- a/lib/filter_nb.c +++ b/lib/filter_nb.c @@ -17,23 +17,6 @@ #include "lib/plist_int.h" #include "lib/routemap.h" -/* Helper function. */ -static void acl_notify_route_map(struct access_list *acl, int route_map_event) -{ - switch (route_map_event) { - case RMAP_EVENT_FILTER_ADDED: - if (acl->master->add_hook) - (*acl->master->add_hook)(acl); - break; - case RMAP_EVENT_FILTER_DELETED: - if (acl->master->delete_hook) - (*acl->master->delete_hook)(acl); - break; - } - - route_map_notify_dependencies(acl->name, route_map_event); -} - static enum nb_error prefix_list_length_validate(struct nb_cb_modify_args *args) { int type = yang_dnode_get_enum(args->dnode, "../../type"); @@ -575,6 +558,15 @@ static int lib_access_list_entry_destroy(struct nb_cb_destroy_args *args) return NB_OK; } +static void +lib_access_list_entry_apply_finish(struct nb_cb_apply_finish_args *args) +{ + struct filter *f; + + f = nb_running_get_entry(args->dnode, NULL, true); + access_list_filter_update(f->acl); +} + /* * XPath: /frr-filter:lib/access-list/entry/action */ @@ -594,8 +586,6 @@ lib_access_list_entry_action_modify(struct nb_cb_modify_args *args) else f->type = FILTER_DENY; - acl_notify_route_map(f->acl, RMAP_EVENT_FILTER_ADDED); - return NB_OK; } @@ -629,8 +619,6 @@ lib_access_list_entry_ipv4_prefix_modify(struct nb_cb_modify_args *args) fz = &f->u.zfilter; yang_dnode_get_prefix(&fz->prefix, args->dnode, NULL); - acl_notify_route_map(f->acl, RMAP_EVENT_FILTER_ADDED); - return NB_OK; } @@ -647,8 +635,6 @@ lib_access_list_entry_ipv4_prefix_destroy(struct nb_cb_destroy_args *args) fz = &f->u.zfilter; memset(&fz->prefix, 0, sizeof(fz->prefix)); - acl_notify_route_map(f->acl, RMAP_EVENT_FILTER_DELETED); - return NB_OK; } @@ -681,8 +667,6 @@ lib_access_list_entry_ipv4_exact_match_modify(struct nb_cb_modify_args *args) fz = &f->u.zfilter; fz->exact = yang_dnode_get_bool(args->dnode, NULL); - acl_notify_route_map(f->acl, RMAP_EVENT_FILTER_ADDED); - return NB_OK; } @@ -699,8 +683,6 @@ lib_access_list_entry_ipv4_exact_match_destroy(struct nb_cb_destroy_args *args) fz = &f->u.zfilter; fz->exact = 0; - acl_notify_route_map(f->acl, RMAP_EVENT_FILTER_DELETED); - return NB_OK; } @@ -733,8 +715,6 @@ lib_access_list_entry_host_modify(struct nb_cb_modify_args *args) yang_dnode_get_ipv4(&fc->addr, args->dnode, NULL); fc->addr_mask.s_addr = CISCO_BIN_HOST_WILDCARD_MASK; - acl_notify_route_map(f->acl, RMAP_EVENT_FILTER_ADDED); - return NB_OK; } @@ -751,8 +731,6 @@ lib_access_list_entry_host_destroy(struct nb_cb_destroy_args *args) fc = &f->u.cfilter; cisco_unset_addr_mask(&fc->addr, &fc->addr_mask); - acl_notify_route_map(f->acl, RMAP_EVENT_FILTER_DELETED); - return NB_OK; } @@ -777,8 +755,6 @@ static int lib_access_list_entry_network_destroy(struct nb_cb_destroy_args *args fc = &f->u.cfilter; cisco_unset_addr_mask(&fc->addr, &fc->addr_mask); - acl_notify_route_map(f->acl, RMAP_EVENT_FILTER_DELETED); - return NB_OK; } @@ -810,8 +786,6 @@ lib_access_list_entry_network_address_modify(struct nb_cb_modify_args *args) fc = &f->u.cfilter; yang_dnode_get_ipv4(&fc->addr, args->dnode, NULL); - acl_notify_route_map(f->acl, RMAP_EVENT_FILTER_ADDED); - return NB_OK; } @@ -843,8 +817,6 @@ lib_access_list_entry_network_mask_modify(struct nb_cb_modify_args *args) fc = &f->u.cfilter; yang_dnode_get_ipv4(&fc->addr_mask, args->dnode, NULL); - acl_notify_route_map(f->acl, RMAP_EVENT_FILTER_ADDED); - return NB_OK; } @@ -877,8 +849,6 @@ lib_access_list_entry_source_any_create(struct nb_cb_create_args *args) fc->addr.s_addr = INADDR_ANY; fc->addr_mask.s_addr = CISCO_BIN_ANY_WILDCARD_MASK; - acl_notify_route_map(f->acl, RMAP_EVENT_FILTER_ADDED); - return NB_OK; } @@ -895,8 +865,6 @@ lib_access_list_entry_source_any_destroy(struct nb_cb_destroy_args *args) fc = &f->u.cfilter; cisco_unset_addr_mask(&fc->addr, &fc->addr_mask); - acl_notify_route_map(f->acl, RMAP_EVENT_FILTER_DELETED); - return NB_OK; } @@ -929,8 +897,6 @@ static int lib_access_list_entry_destination_host_modify( yang_dnode_get_ipv4(&fc->mask, args->dnode, NULL); fc->mask_mask.s_addr = CISCO_BIN_HOST_WILDCARD_MASK; - acl_notify_route_map(f->acl, RMAP_EVENT_FILTER_ADDED); - return NB_OK; } @@ -948,8 +914,6 @@ static int lib_access_list_entry_destination_host_destroy( fc->extended = 0; cisco_unset_addr_mask(&fc->mask, &fc->mask_mask); - acl_notify_route_map(f->acl, RMAP_EVENT_FILTER_DELETED); - return NB_OK; } @@ -977,8 +941,6 @@ static int lib_access_list_entry_destination_network_destroy( fc->extended = 0; cisco_unset_addr_mask(&fc->mask, &fc->mask_mask); - acl_notify_route_map(f->acl, RMAP_EVENT_FILTER_DELETED); - return NB_OK; } @@ -1010,8 +972,6 @@ static int lib_access_list_entry_destination_network_address_modify( fc->extended = 1; yang_dnode_get_ipv4(&fc->mask, args->dnode, NULL); - acl_notify_route_map(f->acl, RMAP_EVENT_FILTER_ADDED); - return NB_OK; } @@ -1043,8 +1003,6 @@ static int lib_access_list_entry_destination_network_mask_modify( fc->extended = 1; yang_dnode_get_ipv4(&fc->mask_mask, args->dnode, NULL); - acl_notify_route_map(f->acl, RMAP_EVENT_FILTER_ADDED); - return NB_OK; } @@ -1077,8 +1035,6 @@ static int lib_access_list_entry_destination_any_create( fc->mask.s_addr = INADDR_ANY; fc->mask_mask.s_addr = CISCO_BIN_ANY_WILDCARD_MASK; - acl_notify_route_map(f->acl, RMAP_EVENT_FILTER_ADDED); - return NB_OK; } @@ -1096,8 +1052,6 @@ static int lib_access_list_entry_destination_any_destroy( fc->extended = 0; cisco_unset_addr_mask(&fc->mask, &fc->mask_mask); - acl_notify_route_map(f->acl, RMAP_EVENT_FILTER_DELETED); - return NB_OK; } @@ -1144,8 +1098,6 @@ static int lib_access_list_entry_any_create(struct nb_cb_create_args *args) break; } - acl_notify_route_map(f->acl, RMAP_EVENT_FILTER_ADDED); - return NB_OK; } @@ -1161,8 +1113,6 @@ static int lib_access_list_entry_any_destroy(struct nb_cb_destroy_args *args) fz = &f->u.zfilter; fz->prefix.family = AF_UNSPEC; - acl_notify_route_map(f->acl, RMAP_EVENT_FILTER_DELETED); - return NB_OK; } @@ -1652,6 +1602,7 @@ const struct frr_yang_module_info frr_filter_info = { .cbs = { .create = lib_access_list_entry_create, .destroy = lib_access_list_entry_destroy, + .apply_finish = lib_access_list_entry_apply_finish, .cli_cmp = access_list_cmp, .cli_show = access_list_show, } From 0bc2ab8598fa1418d3a45499e4f047622d21d6b4 Mon Sep 17 00:00:00 2001 From: Igor Ryzhov Date: Mon, 26 Feb 2024 01:00:17 +0200 Subject: [PATCH 2/2] lib: fix prefix-list entry update When a prefix-list entry is updated, current NB code calls the replacement code multiple times, once per each updated field. It means that when multiple fields of an entry are changed in a single commit, the replacement is done with an interim state of a prefix-list instead of a final one. To fix the issue, we should call the replacement code once, after all fields of an entry are updated. Signed-off-by: Igor Ryzhov --- lib/filter_nb.c | 45 +++++++++++++++++---------------------------- 1 file changed, 17 insertions(+), 28 deletions(-) diff --git a/lib/filter_nb.c b/lib/filter_nb.c index 91f80dcb5e86..39042d39ab9a 100644 --- a/lib/filter_nb.c +++ b/lib/filter_nb.c @@ -136,9 +136,6 @@ static int lib_prefix_list_entry_prefix_length_greater_or_equal_modify( ple->ge = yang_dnode_get_uint8(args->dnode, NULL); - /* Finish prefix entry update procedure. */ - prefix_list_entry_update_finish(ple); - return NB_OK; } @@ -157,9 +154,6 @@ static int lib_prefix_list_entry_prefix_length_lesser_or_equal_modify( ple->le = yang_dnode_get_uint8(args->dnode, NULL); - /* Finish prefix entry update procedure. */ - prefix_list_entry_update_finish(ple); - return NB_OK; } @@ -178,9 +172,6 @@ static int lib_prefix_list_entry_prefix_length_greater_or_equal_destroy( ple->ge = 0; - /* Finish prefix entry update procedure. */ - prefix_list_entry_update_finish(ple); - return NB_OK; } @@ -199,9 +190,6 @@ static int lib_prefix_list_entry_prefix_length_lesser_or_equal_destroy( ple->le = 0; - /* Finish prefix entry update procedure. */ - prefix_list_entry_update_finish(ple); - return NB_OK; } @@ -1229,6 +1217,22 @@ static int lib_prefix_list_entry_destroy(struct nb_cb_destroy_args *args) return NB_OK; } +static void +lib_prefix_list_entry_apply_finish(struct nb_cb_apply_finish_args *args) +{ + struct prefix_list_entry *ple; + + ple = nb_running_get_entry(args->dnode, NULL, true); + + /* + * Finish prefix entry update procedure. The procedure is started in + * children callbacks. `prefix_list_entry_update_start` can be called + * multiple times if multiple children are modified, but it is actually + * executed only once because of the protection by `ple->installed`. + */ + prefix_list_entry_update_finish(ple); +} + /* * XPath: /frr-filter:lib/prefix-list/entry/action */ @@ -1251,9 +1255,6 @@ static int lib_prefix_list_entry_action_modify(struct nb_cb_modify_args *args) else ple->type = PREFIX_DENY; - /* Finish prefix entry update procedure. */ - prefix_list_entry_update_finish(ple); - return NB_OK; } @@ -1281,10 +1282,6 @@ static int lib_prefix_list_entry_prefix_modify(struct nb_cb_modify_args *args) prefix_copy(&ple->prefix, &p); } - - /* Finish prefix entry update procedure. */ - prefix_list_entry_update_finish(ple); - return NB_OK; } @@ -1302,9 +1299,6 @@ static int lib_prefix_list_entry_prefix_destroy(struct nb_cb_destroy_args *args) memset(&ple->prefix, 0, sizeof(ple->prefix)); - /* Finish prefix entry update procedure. */ - prefix_list_entry_update_finish(ple); - return NB_OK; } @@ -1552,9 +1546,6 @@ static int lib_prefix_list_entry_any_create(struct nb_cb_create_args *args) break; } - /* Finish prefix entry update procedure. */ - prefix_list_entry_update_finish(ple); - return NB_OK; } @@ -1572,9 +1563,6 @@ static int lib_prefix_list_entry_any_destroy(struct nb_cb_destroy_args *args) ple->any = false; - /* Finish prefix entry update procedure. */ - prefix_list_entry_update_finish(ple); - return NB_OK; } @@ -1741,6 +1729,7 @@ const struct frr_yang_module_info frr_filter_info = { .cbs = { .create = lib_prefix_list_entry_create, .destroy = lib_prefix_list_entry_destroy, + .apply_finish = lib_prefix_list_entry_apply_finish, .cli_cmp = prefix_list_cmp, .cli_show = prefix_list_show, }