From acd7aea00ee6d23bff4ebb8bb0e7ab6a32874c9b Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Tue, 16 May 2023 05:54:05 -0400 Subject: [PATCH] mgmtd: simplify early config build removing unused code Signed-off-by: Christian Hopps --- mgmtd/mgmt_be_adapter.c | 17 ++-- mgmtd/mgmt_ds.c | 166 +++++++++++----------------------------- mgmtd/mgmt_ds.h | 44 +---------- mgmtd/mgmt_txn.c | 28 ++++--- 4 files changed, 74 insertions(+), 181 deletions(-) diff --git a/mgmtd/mgmt_be_adapter.c b/mgmtd/mgmt_be_adapter.c index e58fb9d184..ab8593b9d2 100644 --- a/mgmtd/mgmt_be_adapter.c +++ b/mgmtd/mgmt_be_adapter.c @@ -602,8 +602,8 @@ static void mgmt_be_adapter_process_msg(uint8_t version, uint8_t *data, mgmtd__be_message__free_unpacked(be_msg, NULL); } -static void mgmt_be_iter_and_get_cfg(struct mgmt_ds_ctx *ds_ctx, char *xpath, - struct lyd_node *node, +static void mgmt_be_iter_and_get_cfg(struct mgmt_ds_ctx *ds_ctx, + const char *xpath, struct lyd_node *node, struct nb_node *nb_node, void *ctx) { struct mgmt_be_client_subscr_info subscr_info; @@ -784,19 +784,24 @@ int mgmt_be_get_adapter_config(struct mgmt_be_client_adapter *adapter, struct mgmt_ds_ctx *ds_ctx, struct nb_config_cbs **cfg_chgs) { - char base_xpath[] = "/"; struct mgmt_be_get_adapter_config_params parms; assert(cfg_chgs); + /* + * TODO: we should be consider making this an assertable condition and + * guaranteeing it be true when this function is called. B/c what is + * going to happen if there are some changes being sent, and we don't + * gather a new snapshot, what new changes that came after the previous + * snapshot will then be lost? + */ if (RB_EMPTY(nb_config_cbs, &adapter->cfg_chgs)) { parms.adapter = adapter; parms.cfg_chgs = &adapter->cfg_chgs; parms.seq = 0; - mgmt_ds_iter_data(ds_ctx, base_xpath, - mgmt_be_iter_and_get_cfg, (void *)&parms, - false); + mgmt_ds_iter_data(ds_ctx, "", mgmt_be_iter_and_get_cfg, + (void *)&parms); } *cfg_chgs = &adapter->cfg_chgs; diff --git a/mgmtd/mgmt_ds.c b/mgmtd/mgmt_ds.c index f9c9933ace..3fd47862b2 100644 --- a/mgmtd/mgmt_ds.c +++ b/mgmtd/mgmt_ds.c @@ -312,36 +312,28 @@ struct nb_config *mgmt_ds_get_nb_config(struct mgmt_ds_ctx *ds_ctx) } static int mgmt_walk_ds_nodes( - struct mgmt_ds_ctx *ds_ctx, char *base_xpath, + struct mgmt_ds_ctx *ds_ctx, const char *base_xpath, struct lyd_node *base_dnode, - void (*mgmt_ds_node_iter_fn)(struct mgmt_ds_ctx *ds_ctx, char *xpath, - struct lyd_node *node, + void (*mgmt_ds_node_iter_fn)(struct mgmt_ds_ctx *ds_ctx, + const char *xpath, struct lyd_node *node, struct nb_node *nb_node, void *ctx), - void *ctx, char *xpaths[], int *num_nodes, bool childs_as_well, - bool alloc_xp_copy) + void *ctx) { - uint32_t indx; - char *xpath, *xpath_buf, *iter_xp; - int ret, num_left = 0, num_found = 0; + /* this is 1k per recursion... */ + char xpath[MGMTD_MAX_XPATH_LEN]; struct lyd_node *dnode; struct nb_node *nbnode; - bool alloc_xp = false; + int ret = 0; - if (xpaths) - assert(num_nodes); + assert(mgmt_ds_node_iter_fn); - if (num_nodes && !*num_nodes) - return 0; - - if (num_nodes) { - num_left = *num_nodes; - MGMTD_DS_DBG(" -- START: num_left:%d", num_left); - *num_nodes = 0; - } - - MGMTD_DS_DBG(" -- START: Base: '%s'", base_xpath); + MGMTD_DS_DBG(" -- START: base xpath: '%s'", base_xpath); if (!base_dnode) + /* + * This function only returns the first node of a possible set + * of matches issuing a warning if more than 1 matches + */ base_dnode = yang_dnode_get( ds_ctx->config_ds ? ds_ctx->root.cfg_root->dnode : ds_ctx->root.dnode_root, @@ -349,111 +341,42 @@ static int mgmt_walk_ds_nodes( if (!base_dnode) return -1; - if (mgmt_ds_node_iter_fn) { - /* - * In case the caller is interested in getting a copy - * of the xpath for themselves (by setting - * 'alloc_xp_copy' to 'true') we make a copy for the - * caller and pass it. Else we pass the original xpath - * buffer. - * - * NOTE: In such case caller will have to take care of - * the copy later. - */ - iter_xp = alloc_xp_copy ? strdup(base_xpath) : base_xpath; + MGMTD_DS_DBG(" search base schema: '%s'", + lysc_path(base_dnode->schema, LYSC_PATH_LOG, xpath, + sizeof(xpath))); - nbnode = (struct nb_node *)base_dnode->schema->priv; - (*mgmt_ds_node_iter_fn)(ds_ctx, iter_xp, base_dnode, nbnode, - ctx); - } - - if (num_nodes) { - (*num_nodes)++; - num_left--; - } + nbnode = (struct nb_node *)base_dnode->schema->priv; + (*mgmt_ds_node_iter_fn)(ds_ctx, base_xpath, base_dnode, nbnode, ctx); /* - * If the base_xpath points to a leaf node, or we don't need to - * visit any children we can skip the tree walk. + * If the base_xpath points to a leaf node we can skip the tree walk. */ - if (!childs_as_well || base_dnode->schema->nodetype & LYD_NODE_TERM) + if (base_dnode->schema->nodetype & LYD_NODE_TERM) return 0; - indx = 0; + /* + * at this point the xpath matched this container node (or some parent + * and we're wildcard descending now) so by walking it's children we + * continue to change the meaning of an xpath regex to rather be a + * prefix matching path + */ + LY_LIST_FOR (lyd_child(base_dnode), dnode) { assert(dnode->schema && dnode->schema->priv); - xpath = NULL; - if (xpaths) { - if (!xpaths[*num_nodes]) { - alloc_xp = true; - xpaths[*num_nodes] = - (char *)calloc(1, MGMTD_MAX_XPATH_LEN); - } - xpath = lyd_path(dnode, LYD_PATH_STD, - xpaths[*num_nodes], - MGMTD_MAX_XPATH_LEN); - } else { - alloc_xp = true; - xpath_buf = (char *)calloc(1, MGMTD_MAX_XPATH_LEN); - (void) lyd_path(dnode, LYD_PATH_STD, xpath_buf, - MGMTD_MAX_XPATH_LEN); - xpath = xpath_buf; - } + (void)lyd_path(dnode, LYD_PATH_STD, xpath, sizeof(xpath)); - assert(xpath); - MGMTD_DS_DBG(" -- XPATH: %s", xpath); - - if (num_nodes) - num_found = num_left; + MGMTD_DS_DBG(" -- Child xpath: %s", xpath); ret = mgmt_walk_ds_nodes(ds_ctx, xpath, dnode, - mgmt_ds_node_iter_fn, ctx, - xpaths ? &xpaths[*num_nodes] : NULL, - num_nodes ? &num_found : NULL, - childs_as_well, alloc_xp_copy); - - if (num_nodes) { - num_left -= num_found; - (*num_nodes) += num_found; - } - - if (alloc_xp) - free(xpath); - + mgmt_ds_node_iter_fn, ctx); if (ret != 0) break; - - indx++; } + MGMTD_DS_DBG(" -- END: base xpath: '%s'", base_xpath); - if (num_nodes) { - MGMTD_DS_DBG(" -- END: *num_nodes:%d, num_left:%d", *num_nodes, - num_left); - } - - return 0; -} - -int mgmt_ds_lookup_data_nodes(struct mgmt_ds_ctx *ds_ctx, const char *xpath, - char *dxpaths[], int *num_nodes, - bool get_childs_as_well, bool alloc_xp_copy) -{ - char base_xpath[MGMTD_MAX_XPATH_LEN]; - - if (!ds_ctx || !num_nodes) - return -1; - - if (xpath[0] == '.' && xpath[1] == '/') - xpath += 2; - - strlcpy(base_xpath, xpath, sizeof(base_xpath)); - mgmt_remove_trailing_separator(base_xpath, '/'); - - return (mgmt_walk_ds_nodes(ds_ctx, base_xpath, NULL, NULL, NULL, - dxpaths, num_nodes, get_childs_as_well, - alloc_xp_copy)); + return ret; } struct lyd_node *mgmt_ds_find_data_node_by_xpath(struct mgmt_ds_ctx *ds_ctx, @@ -534,13 +457,13 @@ int mgmt_ds_load_config_from_file(struct mgmt_ds_ctx *dst, return 0; } -int mgmt_ds_iter_data(struct mgmt_ds_ctx *ds_ctx, char *base_xpath, +int mgmt_ds_iter_data(struct mgmt_ds_ctx *ds_ctx, const char *base_xpath, void (*mgmt_ds_node_iter_fn)(struct mgmt_ds_ctx *ds_ctx, - char *xpath, + const char *xpath, struct lyd_node *node, struct nb_node *nb_node, void *ctx), - void *ctx, bool alloc_xp_copy) + void *ctx) { int ret = 0; char xpath[MGMTD_MAX_XPATH_LEN]; @@ -550,14 +473,19 @@ int mgmt_ds_iter_data(struct mgmt_ds_ctx *ds_ctx, char *base_xpath, if (!ds_ctx) return -1; - mgmt_remove_trailing_separator(base_xpath, '/'); - strlcpy(xpath, base_xpath, sizeof(xpath)); + mgmt_remove_trailing_separator(xpath, '/'); + + /* + * mgmt_ds_iter_data is the only user of mgmt_walk_ds_nodes other than + * mgmt_walk_ds_nodes itself, so we can modify the API if we would like. + * Oper-state should be kept in mind though for the prefix walk + */ MGMTD_DS_DBG(" -- START DS walk for DSid: %d", ds_ctx->ds_id); /* If the base_xpath is empty then crawl the sibblings */ - if (xpath[0] == '\0') { + if (xpath[0] == 0) { base_dnode = ds_ctx->config_ds ? ds_ctx->root.cfg_root->dnode : ds_ctx->root.dnode_root; @@ -569,14 +497,12 @@ int mgmt_ds_iter_data(struct mgmt_ds_ctx *ds_ctx, char *base_xpath, base_dnode = base_dnode->prev; LY_LIST_FOR (base_dnode, node) { - ret = mgmt_walk_ds_nodes( - ds_ctx, xpath, node, mgmt_ds_node_iter_fn, - ctx, NULL, NULL, true, alloc_xp_copy); + ret = mgmt_walk_ds_nodes(ds_ctx, xpath, node, + mgmt_ds_node_iter_fn, ctx); } } else ret = mgmt_walk_ds_nodes(ds_ctx, xpath, base_dnode, - mgmt_ds_node_iter_fn, ctx, NULL, NULL, - true, alloc_xp_copy); + mgmt_ds_node_iter_fn, ctx); return ret; } diff --git a/mgmtd/mgmt_ds.h b/mgmtd/mgmt_ds.h index 2a32eb641a..e5c88742dd 100644 --- a/mgmtd/mgmt_ds.h +++ b/mgmtd/mgmt_ds.h @@ -217,39 +217,6 @@ extern int mgmt_ds_copy_dss(struct mgmt_ds_ctx *src_ds_ctx, */ extern struct nb_config *mgmt_ds_get_nb_config(struct mgmt_ds_ctx *ds_ctx); -/* - * Lookup YANG data nodes. - * - * ds_ctx - * Datastore context. - * - * xpath - * YANG base xpath. - * - * dxpaths - * Out param - array of YANG data xpaths. - * - * num_nodes - * In-out param - number of YANG data xpaths. - * Note - Caller should init this to the size of the array - * provided in dxpaths. - * On return this will have the actual number of xpaths - * being returned. - * - * get_childs_as_well - * TRUE if child nodes needs to be fetched as well, FALSE otherwise. - * - * alloc_xp_copy - * TRUE if the caller is interested in getting a copy of the xpath. - * - * Returns: - * 0 on success, -1 on failure. - */ -extern int mgmt_ds_lookup_data_nodes(struct mgmt_ds_ctx *ds_ctx, - const char *xpath, char *dxpaths[], - int *num_nodes, bool get_childs_as_well, - bool alloc_xp_copy); - /* * Find YANG data node given a datastore handle YANG xpath. */ @@ -281,18 +248,15 @@ extern int mgmt_ds_delete_data_nodes(struct mgmt_ds_ctx *ds_ctx, * be passed to the iterator function provided in * 'iter_fn'. * - * alloc_xp_copy - * TRUE if the caller is interested in getting a copy of the xpath. - * * Returns: * 0 on success, -1 on failure. */ extern int mgmt_ds_iter_data( - struct mgmt_ds_ctx *ds_ctx, char *base_xpath, - void (*mgmt_ds_node_iter_fn)(struct mgmt_ds_ctx *ds_ctx, char *xpath, - struct lyd_node *node, + struct mgmt_ds_ctx *ds_ctx, const char *base_xpath, + void (*mgmt_ds_node_iter_fn)(struct mgmt_ds_ctx *ds_ctx, + const char *xpath, struct lyd_node *node, struct nb_node *nb_node, void *ctx), - void *ctx, bool alloc_xp_copy); + void *ctx); /* * Load config to datastore from a file. diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c index 4b9d95206f..93466a2b39 100644 --- a/mgmtd/mgmt_txn.c +++ b/mgmtd/mgmt_txn.c @@ -1743,10 +1743,10 @@ static void mgmt_txn_send_getcfg_reply_data(struct mgmt_txn_req *txn_req, } static void mgmt_txn_iter_and_send_get_cfg_reply(struct mgmt_ds_ctx *ds_ctx, - char *xpath, - struct lyd_node *node, - struct nb_node *nb_node, - void *ctx) + const char *xpath, + struct lyd_node *node, + struct nb_node *nb_node, + void *ctx) { struct mgmt_txn_req *txn_req; struct mgmt_get_data_req *get_req; @@ -1756,10 +1756,10 @@ static void mgmt_txn_iter_and_send_get_cfg_reply(struct mgmt_ds_ctx *ds_ctx, txn_req = (struct mgmt_txn_req *)ctx; if (!txn_req) - goto mgmtd_ignore_get_cfg_reply_data; + return; if (!(node->schema->nodetype & LYD_NODE_TERM)) - goto mgmtd_ignore_get_cfg_reply_data; + return; assert(txn_req->req_event == MGMTD_TXN_PROC_GETCFG || txn_req->req_event == MGMTD_TXN_PROC_GETDATA); @@ -1771,7 +1771,7 @@ static void mgmt_txn_iter_and_send_get_cfg_reply(struct mgmt_ds_ctx *ds_ctx, data_value = &get_reply->reply_value[get_reply->num_reply]; mgmt_yang_data_init(data); - data->xpath = xpath; + data->xpath = strdup(xpath); mgmt_yang_data_value_init(data_value); data_value->value_case = MGMTD__YANG_DATA_VALUE__VALUE_ENCODED_STR_VAL; data_value->encoded_str_val = (char *)lyd_get_value(node); @@ -1784,12 +1784,6 @@ static void mgmt_txn_iter_and_send_get_cfg_reply(struct mgmt_ds_ctx *ds_ctx, if (get_reply->num_reply == MGMTD_MAX_NUM_DATA_REPLY_IN_BATCH) mgmt_txn_send_getcfg_reply_data(txn_req, get_req); - - return; - -mgmtd_ignore_get_cfg_reply_data: - if (xpath) - free(xpath); } static int mgmt_txn_get_config(struct mgmt_txn_ctx *txn, @@ -1824,10 +1818,14 @@ static int mgmt_txn_get_config(struct mgmt_txn_ctx *txn, MGMTD_TXN_DBG("Trying to get all data under '%s'", get_data->xpaths[indx]); mgmt_init_get_data_reply(get_reply); + /* + * mgmt_ds_iter_data works on path prefixes, but the user may + * want to also use an xpath regexp we need to add this + * functionality. + */ if (mgmt_ds_iter_data(get_data->ds_ctx, get_data->xpaths[indx], mgmt_txn_iter_and_send_get_cfg_reply, - (void *)txn_req, true) - == -1) { + (void *)txn_req) == -1) { MGMTD_TXN_DBG("Invalid Xpath '%s", get_data->xpaths[indx]); mgmt_fe_send_get_cfg_reply(