Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mgmtd: Revert changes related to local validation. #11166

Merged
merged 1 commit into from
Jun 7, 2022
Merged

mgmtd: Revert changes related to local validation. #11166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 47 additions & 2 deletions lib/northbound.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,9 @@ static int nb_node_new_cb(const struct lysc_node *snode, void *arg)
{
struct nb_node *nb_node;
struct lysc_node *sparent, *sparent_list;
struct frr_yang_module_info *module;

module = (struct frr_yang_module_info *)arg;
nb_node = XCALLOC(MTYPE_NB_NODE, sizeof(*nb_node));
yang_snode_get_path(snode, YANG_PATH_DATA, nb_node->xpath,
sizeof(nb_node->xpath));
Expand Down Expand Up @@ -141,6 +143,9 @@ static int nb_node_new_cb(const struct lysc_node *snode, void *arg)
assert(snode->priv == NULL);
((struct lysc_node *)snode)->priv = nb_node;

if (module && module->ignore_cbs)
SET_FLAG(nb_node->flags, F_NB_NODE_IGNORE_CBS);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these nodes even created in the first place if the module supports no callbacks? If they serve no purpose this code should be changed to not create the nb_nodes and all the rest of the state etc that goes along with them.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for the Mgmtd deamon where we need the nb_nodes to populate config data trees and the corresponding schema. This is useful for yang schema-based validations and for fetching the equivalent of "show running config".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Libyang2 has constructed it's own tree for validation and the output of "show running config". In fact the nb_node's hang off a "priv" pointer in the corresponding libyang2 schema node.

Copy link
Contributor

@pushpasis pushpasis May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@choppsv1 Most of the code (including the API needed to do yang-validation) in lib/northbound,c needs a nb_node as parameter. Hence it is going to be lot difficult to avoid creating the nb_nodes on MGMTd. Hence added this new flag on the nb_nodes and then used it to avoid calling callbacks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to revist this later. We should be able to do validation w/o requiring a bunch of dummy nodes.

return YANG_ITER_CONTINUE;
}

Expand Down Expand Up @@ -243,6 +248,9 @@ static unsigned int nb_node_validate_cbs(const struct nb_node *nb_node)
{
unsigned int error = 0;

if (CHECK_FLAG(nb_node->flags, F_NB_NODE_IGNORE_CBS))
return error;

error += nb_node_validate_cb(nb_node, NB_OP_CREATE,
!!nb_node->cbs.create, false);
error += nb_node_validate_cb(nb_node, NB_OP_MODIFY,
Expand Down Expand Up @@ -1213,6 +1221,8 @@ static int nb_callback_create(struct nb_context *context,
bool unexpected_error = false;
int ret;

assert(!CHECK_FLAG(nb_node->flags, F_NB_NODE_IGNORE_CBS));

nb_log_config_callback(event, NB_OP_CREATE, dnode);

args.context = context;
Expand Down Expand Up @@ -1262,6 +1272,8 @@ static int nb_callback_modify(struct nb_context *context,
bool unexpected_error = false;
int ret;

assert(!CHECK_FLAG(nb_node->flags, F_NB_NODE_IGNORE_CBS));

nb_log_config_callback(event, NB_OP_MODIFY, dnode);

args.context = context;
Expand Down Expand Up @@ -1311,6 +1323,8 @@ static int nb_callback_destroy(struct nb_context *context,
bool unexpected_error = false;
int ret;

assert(!CHECK_FLAG(nb_node->flags, F_NB_NODE_IGNORE_CBS));

nb_log_config_callback(event, NB_OP_DESTROY, dnode);

args.context = context;
Expand Down Expand Up @@ -1354,6 +1368,8 @@ static int nb_callback_move(struct nb_context *context,
bool unexpected_error = false;
int ret;

assert(!CHECK_FLAG(nb_node->flags, F_NB_NODE_IGNORE_CBS));

nb_log_config_callback(event, NB_OP_MOVE, dnode);

args.context = context;
Expand Down Expand Up @@ -1397,6 +1413,9 @@ static int nb_callback_pre_validate(struct nb_context *context,
bool unexpected_error = false;
int ret;

if (CHECK_FLAG(nb_node->flags, F_NB_NODE_IGNORE_CBS))
return 0;

nb_log_config_callback(NB_EV_VALIDATE, NB_OP_PRE_VALIDATE, dnode);

args.dnode = dnode;
Expand Down Expand Up @@ -1428,6 +1447,9 @@ static void nb_callback_apply_finish(struct nb_context *context,
{
struct nb_cb_apply_finish_args args = {};

if (CHECK_FLAG(nb_node->flags, F_NB_NODE_IGNORE_CBS))
return;

nb_log_config_callback(NB_EV_APPLY, NB_OP_APPLY_FINISH, dnode);

args.context = context;
Expand All @@ -1443,6 +1465,9 @@ struct yang_data *nb_callback_get_elem(const struct nb_node *nb_node,
{
struct nb_cb_get_elem_args args = {};

if (CHECK_FLAG(nb_node->flags, F_NB_NODE_IGNORE_CBS))
return NULL;

DEBUGD(&nb_dbg_cbs_state,
"northbound callback (get_elem): xpath [%s] list_entry [%p]",
xpath, list_entry);
Expand All @@ -1458,6 +1483,9 @@ const void *nb_callback_get_next(const struct nb_node *nb_node,
{
struct nb_cb_get_next_args args = {};

if (CHECK_FLAG(nb_node->flags, F_NB_NODE_IGNORE_CBS))
return NULL;

DEBUGD(&nb_dbg_cbs_state,
"northbound callback (get_next): node [%s] parent_list_entry [%p] list_entry [%p]",
nb_node->xpath, parent_list_entry, list_entry);
Expand All @@ -1472,6 +1500,9 @@ int nb_callback_get_keys(const struct nb_node *nb_node, const void *list_entry,
{
struct nb_cb_get_keys_args args = {};

if (CHECK_FLAG(nb_node->flags, F_NB_NODE_IGNORE_CBS))
return 0;

DEBUGD(&nb_dbg_cbs_state,
"northbound callback (get_keys): node [%s] list_entry [%p]",
nb_node->xpath, list_entry);
Expand All @@ -1487,6 +1518,9 @@ const void *nb_callback_lookup_entry(const struct nb_node *nb_node,
{
struct nb_cb_lookup_entry_args args = {};

if (CHECK_FLAG(nb_node->flags, F_NB_NODE_IGNORE_CBS))
return NULL;

DEBUGD(&nb_dbg_cbs_state,
"northbound callback (lookup_entry): node [%s] parent_list_entry [%p]",
nb_node->xpath, parent_list_entry);
Expand All @@ -1502,6 +1536,9 @@ int nb_callback_rpc(const struct nb_node *nb_node, const char *xpath,
{
struct nb_cb_rpc_args args = {};

if (CHECK_FLAG(nb_node->flags, F_NB_NODE_IGNORE_CBS))
return 0;

DEBUGD(&nb_dbg_cbs_rpc, "northbound RPC: %s", xpath);

args.xpath = xpath;
Expand All @@ -1528,6 +1565,9 @@ static int nb_callback_configuration(struct nb_context *context,
union nb_resource *resource;
int ret = NB_ERR;

if (CHECK_FLAG(nb_node->flags, F_NB_NODE_IGNORE_CBS))
return NB_OK;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check here is done prior to calling nb_callback_{create,modify,destroy,move}, then in each of those called functions the check is done again. That secpmd redundant check should just be an assert.

However, I don't know why this code is even being invoked in the first place for a module with no callbacks -- as mentioned in an earlier comment. This comment applies to the other top level checks (pre_validate, apply_finish etc..)

Copy link
Contributor

@pushpasis pushpasis May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nb_init() which is invoked by frr_init calls nb_validate_callbacks() for all FRR daemons. These changes tries to avoid that for YANG modules that are marked with 'ignore_cbs'.


if (event == NB_EV_VALIDATE)
resource = NULL;
else
Expand Down Expand Up @@ -1925,7 +1965,7 @@ static int nb_oper_data_iter_list(const struct nb_node *nb_node,
/* Iterate over all list entries. */
do {
const struct lysc_node_leaf *skey;
struct yang_list_keys list_keys;
struct yang_list_keys list_keys = {};
char xpath[XPATH_MAXLEN * 2];
int ret;

Expand Down Expand Up @@ -2585,6 +2625,10 @@ const char *nb_client_name(enum nb_client client)

static void nb_load_callbacks(const struct frr_yang_module_info *module)
{

if (module->ignore_cbs)
return;

for (size_t i = 0; module->nodes[i].xpath; i++) {
struct nb_node *nb_node;
uint32_t priority;
Expand Down Expand Up @@ -2658,7 +2702,8 @@ void nb_init(struct thread_master *tm,

/* Initialize the compiled nodes with northbound data */
for (size_t i = 0; i < nmodules; i++) {
yang_snodes_iterate(loaded[i]->info, nb_node_new_cb, 0, NULL);
yang_snodes_iterate(loaded[i]->info, nb_node_new_cb, 0,
(void *)modules[i]);
nb_load_callbacks(modules[i]);
}

Expand Down
8 changes: 8 additions & 0 deletions lib/northbound.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,8 @@ struct nb_node {
#define F_NB_NODE_CONFIG_ONLY 0x01
/* The YANG list doesn't contain key leafs. */
#define F_NB_NODE_KEYLESS_LIST 0x02
/* Ignore callbacks for this node */
#define F_NB_NODE_IGNORE_CBS 0x04

/*
* HACK: old gcc versions (< 5.x) have a bug that prevents C99 flexible arrays
Expand All @@ -622,6 +624,12 @@ struct frr_yang_module_info {
/* YANG module name. */
const char *name;

/*
* Ignore callbacks for this module. Set this to true to
* load module without any callbacks.
*/
bool ignore_cbs;

/* Northbound callbacks. */
const struct {
/* Data path of this YANG node. */
Expand Down
16 changes: 15 additions & 1 deletion mgmtd/mgmt_be_adapter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,10 +124,24 @@ static struct mgmt_be_adapter_list_head mgmt_be_adapters;
static struct mgmt_be_client_adapter
*mgmt_be_adapters_by_id[MGMTD_BE_CLIENT_ID_MAX];


/*
* List of YANG modules for every Backend Clients gets added here.
* These will typically have to be prototyped in mgmtd_be_adapter.h
* and then added under mgmt_yang_modules[] in mgmt_main.c.
*
* NOTE: The .ignore_cbs for all entries to be set to 'true' to
* avoid validating backend northbound callback loading.
*/
const struct frr_yang_module_info frr_mgmt_staticd_info = {
.name = "frr-staticd",
.ignore_cbs = true
};

/* Forward declarations */
static void
mgmt_be_adapter_register_event(struct mgmt_be_client_adapter *adapter,
enum mgmt_be_event event);
enum mgmt_be_event event);

static struct mgmt_be_client_adapter *
mgmt_be_find_adapter_by_fd(int conn_fd)
Expand Down
10 changes: 8 additions & 2 deletions mgmtd/mgmt_be_adapter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,16 @@ union mgmt_be_xpath_subscr_info {
};

struct mgmt_be_client_subscr_info {
union mgmt_be_xpath_subscr_info
xpath_subscr[MGMTD_BE_CLIENT_ID_MAX];
union mgmt_be_xpath_subscr_info xpath_subscr[MGMTD_BE_CLIENT_ID_MAX];
};

/*
* NOTE: List of YANG modules for every Backend Clients gets added here.
* These will typically have to be defined in mgmtd_be_adapter.c file
* first.
*/
extern const struct frr_yang_module_info frr_mgmt_staticd_info;

/* Initialise backend adapter module. */
extern int mgmt_be_adapter_init(struct thread_master *tm);

Expand Down
2 changes: 1 addition & 1 deletion mgmtd/mgmt_main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ static void mgmt_vrf_terminate(void)
*/
static const struct frr_yang_module_info *const mgmt_yang_modules[] = {
&frr_filter_info, &frr_interface_info, &frr_route_map_info,
&frr_routing_info, &frr_vrf_info, &frr_staticd_info,
&frr_routing_info, &frr_vrf_info, &frr_mgmt_staticd_info,
};

FRR_DAEMON_INFO(mgmtd, MGMTD, .vty_port = MGMTD_VTY_PORT,
Expand Down
6 changes: 2 additions & 4 deletions mgmtd/subdir.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,9 @@ lib_LTLIBRARIES += mgmtd/libmgmt_be_nb.la
nodist_mgmtd_libmgmt_be_nb_la_SOURCES = \
yang/frr-staticd.yang.c \
staticd/static_vty.c \
staticd/static_nb.c \
staticd/static_nb_config.c \
# end
mgmtd_libmgmt_be_nb_la_CFLAGS = $(AM_CFLAGS) -DINCLUDE_MGMTD_CMDDEFS_ONLY -DINCLUDE_MGMTD_VALIDATE_ONLY
mgmtd_libmgmt_be_nb_la_CPPFLAGS = $(AM_CPPFLAGS) -DINCLUDE_MGMTD_CMDDEFS_ONLY -DINCLUDE_MGMTD_VALIDATE_ONLY
mgmtd_libmgmt_be_nb_la_CFLAGS = $(AM_CFLAGS) -DINCLUDE_MGMTD_CMDDEFS_ONLY
mgmtd_libmgmt_be_nb_la_CPPFLAGS = $(AM_CPPFLAGS) -DINCLUDE_MGMTD_CMDDEFS_ONLY
mgmtd_libmgmt_be_nb_la_LDFLAGS = -version-info 0:0:0

noinst_LIBRARIES += mgmtd/libmgmtd.a
Expand Down
4 changes: 0 additions & 4 deletions staticd/static_nb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,7 @@ const struct frr_yang_module_info frr_staticd_info = {
{
.xpath = "/frr-routing:routing/control-plane-protocols/control-plane-protocol/frr-staticd:staticd/route-list/path-list/frr-nexthops/nexthop",
.cbs = {
#ifndef INCLUDE_MGMTD_VALIDATE_ONLY
.apply_finish = routing_control_plane_protocols_control_plane_protocol_staticd_route_list_path_list_frr_nexthops_nexthop_apply_finish,
#endif /* ifndef INCLUDE_MGMTD_VALIDATE_ONLY */
.create = routing_control_plane_protocols_control_plane_protocol_staticd_route_list_path_list_frr_nexthops_nexthop_create,
.destroy = routing_control_plane_protocols_control_plane_protocol_staticd_route_list_path_list_frr_nexthops_nexthop_destroy,
.pre_validate = routing_control_plane_protocols_control_plane_protocol_staticd_route_list_path_list_frr_nexthops_nexthop_pre_validate,
Expand Down Expand Up @@ -143,9 +141,7 @@ const struct frr_yang_module_info frr_staticd_info = {
{
.xpath = "/frr-routing:routing/control-plane-protocols/control-plane-protocol/frr-staticd:staticd/route-list/src-list/path-list/frr-nexthops/nexthop",
.cbs = {
#ifndef INCLUDE_MGMTD_VALIDATE_ONLY
.apply_finish = routing_control_plane_protocols_control_plane_protocol_staticd_route_list_src_list_path_list_frr_nexthops_nexthop_apply_finish,
#endif /* ifndef INCLUDE_MGMTD_VALIDATE_ONLY */
.create = routing_control_plane_protocols_control_plane_protocol_staticd_route_list_src_list_path_list_frr_nexthops_nexthop_create,
.destroy = routing_control_plane_protocols_control_plane_protocol_staticd_route_list_src_list_path_list_frr_nexthops_nexthop_destroy,
.pre_validate = routing_control_plane_protocols_control_plane_protocol_staticd_route_list_path_list_frr_nexthops_nexthop_pre_validate,
Expand Down
Loading