From f8de2b7c0a1e971fb5755ebd858188109a66ce69 Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Thu, 1 Feb 2024 20:14:10 +0100 Subject: [PATCH] Optimized datastore access by ensuring REPORT_ALL in memory and EXPLICIT in file --- CHANGELOG.md | 1 + apps/backend/backend_get.c | 227 ++++++++++----------------- apps/backend/backend_plugin.c | 1 - apps/backend/backend_startup.c | 3 + apps/backend/clixon_backend_plugin.h | 3 +- lib/clixon/clixon_datastore.h | 5 +- lib/src/clixon_datastore.c | 40 ++++- lib/src/clixon_datastore_read.c | 92 ++++------- lib/src/clixon_datastore_write.c | 19 ++- lib/src/clixon_nacm.c | 3 +- lib/src/clixon_xml_map.c | 5 +- test/test_nacm_datanode_read.sh | 1 + 12 files changed, 183 insertions(+), 217 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d4f880df..c8111bda 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ Expected: February 2024 * All clixon applications added command-line option `-V` for printing version * New ca_version callback for customized version output * Optimization: + * Optimized datastore access by ensuring REPORT_ALL in memory and EXPLICIT in file * Added mountpoint cache as yang flag `YANG_FLAG_MTPOINT_POTENTIAL` * Optimized `yang_find`, especially namespace lookup * Filtered state data if not match xpath diff --git a/apps/backend/backend_get.c b/apps/backend/backend_get.c index 0dcad562..c71173b8 100644 --- a/apps/backend/backend_get.c +++ b/apps/backend/backend_get.c @@ -120,7 +120,8 @@ restconf_client_get_capabilities(clixon_handle h, * * @param[in] h Clixon handle * @param[in] yspec Yang spec - * @param[in] xpath Xpath selection, not used but may be to filter early + * @param[in] xpath XPath selection, not used but may be to filter early + * @param[in] nsc Namespace context * @param[in] module Name of yang module * @param[in] top Top symbol, ie netconf or restconf-state * @param[in,out] xret Existing XML tree, merge x into this @@ -132,15 +133,14 @@ static int client_get_streams(clixon_handle h, yang_stmt *yspec, char *xpath, + cvec *nsc, yang_stmt *ymod, char *top, cxobj **xret) { int retval = -1; yang_stmt *yns = NULL; /* yang namespace */ - cxobj *x = NULL; cbuf *cb = NULL; - int ret; if ((yns = yang_find(ymod, Y_NAMESPACE, NULL)) == NULL){ clixon_err(OE_YANG, 0, "%s yang namespace not found", yang_argument_get(ymod)); @@ -158,21 +158,15 @@ client_get_streams(clixon_handle h, goto done; cprintf(cb,"", top); - if (clixon_xml_parse_string(cbuf_get(cb), YB_MODULE, yspec, &x, NULL) < 0){ + if (clixon_xml_parse_string(cbuf_get(cb), YB_MODULE, yspec, xret, NULL) < 0){ if (xret && netconf_operation_failed_xml(xret, "protocol", clixon_err_reason())< 0) goto done; goto fail; } - if ((ret = netconf_trymerge(x, yspec, xret)) < 0) - goto done; - if (ret == 0) - goto fail; retval = 1; done: if (cb) cbuf_free(cb); - if (x) - xml_free(x); return retval; fail: retval = 0; @@ -184,7 +178,6 @@ client_get_streams(clixon_handle h, * @param[in] h Clixon handle * @param[in] xpath XPath selection, may be used to filter early * @param[in] nsc XML Namespace context for xpath - * @param[in] wdef With-defaults parameter, see RFC 6243 * @param[in,out] xret Existing XML tree, merge x into this, or rpc-error * @retval 1 OK * @retval 0 Statedata callback failed (error in xret) @@ -199,14 +192,11 @@ client_get_streams(clixon_handle h, * Instead, I think there should be a second out argument **xerr with the error message, see code * for CLICON_NETCONF_MONITORING which is transformed in calling function(?) to an internal error * message. But this needs to be explored in all sub-functions - * - * */ static int get_statedata(clixon_handle h, char *xpath, cvec *nsc, - withdefaults_type wdef, cxobj **xret) { int retval = -1; @@ -214,7 +204,6 @@ get_statedata(clixon_handle h, yang_stmt *ymod; cxobj *x1 = NULL; int ret; - char *namespace; cbuf *cb = NULL; cxobj *xerr = NULL; @@ -232,37 +221,34 @@ get_statedata(clixon_handle h, clixon_err(OE_YANG, ENOENT, "yang module clixon-rfc5277 not found"); goto done; } - if ((namespace = yang_find_mynamespace(ymod)) == NULL){ - clixon_err(OE_YANG, ENOENT, "clixon-rfc5277 namespace not found"); - goto done; - } - cprintf(cb, "", namespace); - if (clixon_xml_parse_string(cbuf_get(cb), YB_MODULE, yspec, xret, NULL) < 0) - goto done; - if ((ret = client_get_streams(h, yspec, xpath, ymod, "netconf", xret)) < 0) + if ((ret = client_get_streams(h, yspec, xpath, nsc, ymod, "netconf", &x1)) < 0) goto done; if (ret == 0) goto fail; + if (xpath_first(x1, nsc, "%s", xpath) != NULL){ + if ((ret = netconf_trymerge(x1, yspec, xret)) < 0) + goto done; + if (ret == 0) + goto fail; + } } if (clicon_option_bool(h, "CLICON_STREAM_DISCOVERY_RFC8040")){ if ((ymod = yang_find_module_by_name(yspec, "ietf-restconf-monitoring")) == NULL){ clixon_err(OE_YANG, ENOENT, "yang module ietf-restconf-monitoring not found"); goto done; } - if ((namespace = yang_find_mynamespace(ymod)) == NULL){ - clixon_err(OE_YANG, ENOENT, "ietf-restconf-monitoring namespace not found"); - goto done; - } - cbuf_reset(cb); - cprintf(cb, "", namespace); - if (clixon_xml_parse_string(cbuf_get(cb), YB_MODULE, yspec, xret, NULL) < 0) - goto done; - if ((ret = client_get_streams(h, yspec, xpath, ymod, "restconf-state", xret)) < 0) + if ((ret = client_get_streams(h, yspec, xpath, nsc, ymod, "restconf-state", &x1)) < 0) goto done; if (ret == 0) goto fail; - if ((ret = restconf_client_get_capabilities(h, yspec, xpath, xret)) < 0) + if (restconf_client_get_capabilities(h, yspec, xpath, &x1) < 0) goto done; + if (xpath_first(x1, nsc, "%s", xpath) != NULL){ + if ((ret = netconf_trymerge(x1, yspec, xret)) < 0) + goto done; + if (ret == 0) + goto fail; + } } if (clicon_option_bool(h, "CLICON_YANG_LIBRARY")){ if ((ret = yang_modules_state_get(h, yspec, xpath, nsc, 0, xret)) < 0) @@ -270,37 +256,42 @@ get_statedata(clixon_handle h, if (ret == 0) goto fail; } - if (clicon_option_bool(h, "CLICON_NETCONF_MONITORING")){ - if ((ret = netconf_monitoring_state_get(h, yspec, xpath, nsc, xret, &xerr)) < 0) - goto done; - if (ret == 0){ - if (clixon_netconf_internal_error(xerr, " . Internal error, netconf_monitoring_state returned invalid XML", NULL) < 0) + if (clicon_option_bool(h, "CLICON_NETCONF_MONITORING")) + if (xpath == NULL || /* Raw optimization of xpath filtering */ + strcmp(xpath, "/") == 0 || + strstr(xpath, "netconf-state") != 0){ + if ((ret = netconf_monitoring_state_get(h, yspec, xpath, nsc, xret, &xerr)) < 0) goto done; - if (*xret) - xml_free(*xret); - *xret = xerr; - xerr = NULL; - goto fail; - } - /* Some state, client state, is avaliable in backend only, not in lib - * Needs merge since same subtree as previous lib state - */ - if ((ret = backend_monitoring_state_get(h, yspec, xpath, nsc, &x1, &xerr)) < 0) - goto done; - if (ret == 0){ - if (clixon_netconf_internal_error(xerr, " . Internal error, baenckend_monitoring_state_get returned invalid XML", NULL) < 0) + if (ret == 0){ + if (clixon_netconf_internal_error(xerr, " . Internal error, netconf_monitoring_state returned invalid XML", NULL) < 0) + goto done; + if (*xret) + xml_free(*xret); + *xret = xerr; + xerr = NULL; + goto fail; + } + /* Some state, client state, is avaliable in backend only, not in lib + * Needs merge since same subtree as previous lib state + */ + if ((ret = backend_monitoring_state_get(h, yspec, xpath, nsc, &x1, &xerr)) < 0) goto done; - if (*xret) - xml_free(*xret); - *xret = xerr; - xerr = NULL; - goto fail; + if (ret == 0){ + if (clixon_netconf_internal_error(xerr, " . Internal error, baenckend_monitoring_state_get returned invalid XML", NULL) < 0) + goto done; + if (*xret) + xml_free(*xret); + *xret = xerr; + xerr = NULL; + goto fail; + } + if (xpath_first(x1, nsc, "%s", xpath) != NULL){ + if ((ret = netconf_trymerge(x1, yspec, xret)) < 0) + goto done; + if (ret == 0) + goto fail; + } } - if ((ret = netconf_trymerge(x1, yspec, xret)) < 0) - goto done; - if (ret == 0) - goto fail; - } if (clicon_option_bool(h, "CLICON_YANG_SCHEMA_MOUNT")){ if ((ret = yang_schema_mount_statedata(h, yspec, xpath, nsc, xret, &xerr)) < 0) goto done; @@ -315,62 +306,10 @@ get_statedata(clixon_handle h, } } /* Use plugin state callbacks */ - if ((ret = clixon_plugin_statedata_all(h, yspec, nsc, xpath, wdef, xret)) < 0) + if ((ret = clixon_plugin_statedata_all(h, yspec, nsc, xpath, xret)) < 0) goto done; if (ret == 0) goto fail; - switch (wdef){ - case WITHDEFAULTS_REPORT_ALL: - case WITHDEFAULTS_EXPLICIT: - /* Add global defaults. */ - if (xml_global_defaults(h, *xret, nsc, xpath, yspec, 1) < 0) - goto done; - /* Apply default values */ - if (xml_default_recurse(*xret, 1) < 0) - goto done; - break; - case WITHDEFAULTS_TRIM: - /* Mark and remove nodes having schema default values */ - if (xml_apply((*xret), CX_ELMNT, (xml_applyfn_t*) xml_flag_state_default_value, (void*) XML_FLAG_MARK) < 0) - goto done; - if (xml_tree_prune_flags((*xret), XML_FLAG_MARK, XML_FLAG_MARK)< 0) - goto done; - if (xml_defaults_nopresence((*xret), 1) < 0) - goto done; - break; - case WITHDEFAULTS_REPORT_ALL_TAGGED:{ - cxobj *xc; - char *ns; - /* Add global defaults. */ - if (xml_global_defaults(h, *xret, nsc, xpath, yspec, 1) < 0) - goto done; - /* Apply default values */ - if (xml_default_recurse(*xret, 1) < 0) - goto done; - xc = NULL; - while ((xc = xml_child_each((*xret), xc, CX_ELMNT)) != NULL){ - /* Check if exists */ - ns = NULL; - if (xml2ns(xc, IETF_NETCONF_WITH_DEFAULTS_ATTR_PREFIX, &ns) < 0) - goto done; - if (ns == NULL){ - if (xmlns_set(xc, IETF_NETCONF_WITH_DEFAULTS_ATTR_PREFIX, IETF_NETCONF_WITH_DEFAULTS_ATTR_NAMESPACE) < 0) - goto done; - } - else if (strcmp(ns, IETF_NETCONF_WITH_DEFAULTS_ATTR_NAMESPACE) != 0){ - /* XXX: Assume if namespace is set that it is withdefaults otherwise just ignore? */ - continue; - } - } - /* Mark nodes having default schema values */ - if (xml_apply(*xret, CX_ELMNT, (xml_applyfn_t*) xml_flag_state_default_value, (void*) XML_FLAG_MARK) < 0) - goto done; - /* Add tag attributes to default nodes */ - if (xml_apply(*xret, CX_ELMNT, (xml_applyfn_t*) xml_add_default_tag, (void*) (XML_FLAG_DEFAULT | XML_FLAG_MARK)) < 0) - goto done; - break; - } - } /* switch wdef */ retval = 1; /* OK */ done: clixon_debug(CLIXON_DBG_CLIENT, "retval:%d", retval); @@ -456,15 +395,16 @@ filter_xpath_again(clixon_handle h, * @retval -1 Error */ static int -get_nacm_and_reply(clixon_handle h, - cxobj *xret, - cxobj **xvec, - size_t xlen, - char *xpath, - cvec *nsc, - char *username, - int32_t depth, - cbuf *cbret) +get_nacm_and_reply(clixon_handle h, + cxobj *xret, + cxobj **xvec, + size_t xlen, + char *xpath, + cvec *nsc, + char *username, + int32_t depth, + withdefaults_type wdef, + cbuf *cbret) { int retval = -1; cxobj *xnacm = NULL; @@ -483,7 +423,7 @@ get_nacm_and_reply(clixon_handle h, if (xml_name_set(xret, NETCONF_OUTPUT_DATA) < 0) goto done; /* Top level is data, so add 1 to depth if significant */ - if (clixon_xml2cbuf(cbret, xret, 0, 0, NULL, depth>0?depth+1:depth, 0) < 0) + if (clixon_xml2cbuf1(cbret, xret, 0, 0, NULL, depth>0?depth+1:depth, 0, wdef) < 0) goto done; } cprintf(cbret, ""); @@ -567,7 +507,7 @@ list_pagination_hdr(clixon_handle h, * @param[in] xpath XPath point to object to get * @param[in] nsc Namespace context of xpath * @param[in] username - * @param[in] wdef With-defaults parameter, see RFC 6243 + * @param[in] wdef With-defaults parameter, see RFC 6243 * @param[out] cbret Return xml tree, eg ..., de_id; clicon_db_elmnt_set(h, db, &de0); /* Content is copied */ + /* Add default global values (to make xpath below include defaults) */ + // Alt: xmldb_populate(h, db) + if (xml_global_defaults(h, x0t, nsc, xpath, yspec, 0) < 0) + goto done; + /* Add default recursive values */ + if (xml_default_recurse(x0t, 0) < 0) + goto done; } /* x0t == NULL */ else x0t = de->de_xml; - if (yb == YB_MODULE && !xml_spec(x0t)){ - /* Seems unneccesary to always do this, but breaks test_plugin_reset.sh if removed */ - if ((ret = xml_bind_yang(h, x0t, YB_MODULE, yspec, xerr)) < 0) - goto done; - if (ret == 1) { - /* Add default global values (to make xpath below include defaults) */ - if (xml_global_defaults(h, x0t, nsc, xpath, yspec, 0) < 0) - goto done; - /* Add default recursive values */ - if (xml_default_recurse(x0t, 0) < 0) - goto done; - } - } /* Here x0t looks like: ... */ /* Given the xpath, return a vector of matches in xvec * Can we do everything in one go? @@ -752,7 +746,7 @@ xmldb_get_cache(clixon_handle h, */ if (xpath_vec(x0t, nsc, "%s", &xvec, &xlen, xpath?xpath:"/") < 0) goto done; - + // XXX: Remove copying and return x0 eventually /* Make new tree by copying top-of-tree from x0t to x1t */ if ((x1t = xml_new(xml_name(x0t), NULL, CX_ELMNT)) == NULL) goto done; @@ -786,51 +780,6 @@ xmldb_get_cache(clixon_handle h, if (xml_apply(x1t, CX_ELMNT, (xml_applyfn_t*)xml_flag_reset, (void*)(XML_FLAG_MARK|XML_FLAG_CHANGE)) < 0) goto done; } - /* Original tree: Remove global defaults and empty non-presence containers */ - if (xml_defaults_nopresence(x0t, 2) < 0) - goto done; - switch (wdef){ - case WITHDEFAULTS_REPORT_ALL: - break; - case WITHDEFAULTS_TRIM: - /* Mark and remove nodes having schema default values */ - if (xml_apply(x1t, CX_ELMNT, (xml_applyfn_t*) xml_flag_default_value, (void*) XML_FLAG_MARK) < 0) - goto done; - if (xml_tree_prune_flags(x1t, XML_FLAG_MARK, XML_FLAG_MARK) < 0) - goto done; - if (xml_defaults_nopresence(x1t, 1) < 0) - goto done; - break; - case WITHDEFAULTS_EXPLICIT: - if (xml_defaults_nopresence(x1t, 2) < 0) - goto done; - break; - case WITHDEFAULTS_REPORT_ALL_TAGGED:{ - cxobj *x; - char *ns; - x = NULL; - while ((x = xml_child_each(x1t, x, CX_ELMNT)) != NULL){ - ns = NULL; - if (xml2ns(x, IETF_NETCONF_WITH_DEFAULTS_ATTR_PREFIX, &ns) < 0) - goto done; - if (ns == NULL){ - if (xmlns_set(x, IETF_NETCONF_WITH_DEFAULTS_ATTR_PREFIX, IETF_NETCONF_WITH_DEFAULTS_ATTR_NAMESPACE) < 0) - goto done; - } - else if (strcmp(ns, IETF_NETCONF_WITH_DEFAULTS_ATTR_NAMESPACE) != 0){ - /* XXX: Assume if namespace is set that it is withdefaults otherwise just ignore? */ - continue; - } - } - /* Mark nodes having default schema values */ - if (xml_apply(x1t, CX_ELMNT, (xml_applyfn_t*) xml_flag_default_value, (void*) XML_FLAG_MARK) < 0) - goto done; - /* Add tag attributes to default nodes */ - if (xml_apply(x1t, CX_ELMNT, (xml_applyfn_t*) xml_add_default_tag, (void*) (XML_FLAG_DEFAULT | XML_FLAG_MARK)) < 0) - goto done; - break; - } - } /* switch wdef */ /* If empty NACM config, then disable NACM if loaded */ if (clicon_option_bool(h, "CLICON_NACM_DISABLED_ON_EMPTY")){ @@ -935,5 +884,26 @@ xmldb_get0(clixon_handle h, modstate_diff_t *msdiff, cxobj **xerr) { - return xmldb_get_cache(h, db, yb, nsc, xpath, wdef, xret, msdiff, xerr); + int retval = -1; + int ret; + cxobj *x = NULL; + + if (wdef != WITHDEFAULTS_EXPLICIT) + return xmldb_get_cache(h, db, yb, nsc, xpath, 0, xret, msdiff, xerr); + if ((ret = xmldb_get_cache(h, db, yb, nsc, xpath, 0, &x, msdiff, xerr)) < 0) + goto done; + if (ret == 0) + goto fail; + if (xml_defaults_nopresence(x, 2) < 0) + goto done; + *xret = x; + x = NULL; + retval = 1; + done: + if (x) + xml_free(x); + return retval; + fail: + retval = 0; + goto done; } diff --git a/lib/src/clixon_datastore_write.c b/lib/src/clixon_datastore_write.c index b2e74b0e..0d7c8cd4 100644 --- a/lib/src/clixon_datastore_write.c +++ b/lib/src/clixon_datastore_write.c @@ -1286,13 +1286,18 @@ xmldb_put(clixon_handle h, if (xml_apply(x0, CX_ELMNT, (xml_applyfn_t*)xml_flag_reset, (void*)(XML_FLAG_NONE|XML_FLAG_MARK)) < 0) goto done; - /* Remove global defaults and empty non-presence containers */ - if (xml_defaults_nopresence(x0, 2) < 0) + /* Remove empty non-presence containers recursively. + * XXX should really be done for only new data in text_modify + */ + if (xml_defaults_nopresence(x0, 3) < 0) + goto done; + /* Complete defaults in incoming x1 + */ + if (xml_global_defaults(h, x0, nsc, "/", yspec, 0) < 0) + goto done; + /* Add default recursive values */ + if (xml_default_recurse(x0, 0) < 0) goto done; -#if 0 /* debug */ - if (xml_apply0(x0, -1, xml_sort_verify, NULL) < 0) - clixon_log(h, LOG_NOTICE, "%s: verify failed #3", __FUNCTION__); -#endif /* Write back to datastore cache if first time */ { db_elmnt de0 = {0,}; @@ -1331,7 +1336,7 @@ xmldb_put(clixon_handle h, if (clixon_json2file(f, x0, pretty, fprintf, 0, 0) < 0) goto done; } - else if (clixon_xml2file(f, x0, 0, pretty, NULL, fprintf, 0, 0) < 0) + else if (clixon_xml2file1(f, x0, 0, pretty, NULL, fprintf, 0, 0, WITHDEFAULTS_EXPLICIT) < 0) goto done; /* Remove modules state after writing to file */ diff --git a/lib/src/clixon_nacm.c b/lib/src/clixon_nacm.c index 2e643796..85d19e39 100644 --- a/lib/src/clixon_nacm.c +++ b/lib/src/clixon_nacm.c @@ -885,8 +885,7 @@ nacm_data_read_xrule_xml(cxobj *xn, * * @param[in] h Clixon handle * @param[in] xn XML node (requested node) - * @param[in] rulevec Precomputed rules that apply to this user group - * @param[in] xpathvec Precomputed xpath results that apply to this XML tree + * @param[in] pv_list Precomputed rules + paths that apply to this user group * @param[in] yspec YANG spec * @retval 0 OK * @retval -1 Error diff --git a/lib/src/clixon_xml_map.c b/lib/src/clixon_xml_map.c index 77e261b4..41ce1bac 100644 --- a/lib/src/clixon_xml_map.c +++ b/lib/src/clixon_xml_map.c @@ -1660,7 +1660,10 @@ xml_copy_marked(cxobj *x0, char *name; char *prefix; - assert(x0 && x1); + if (x0 == NULL || x1 == NULL){ + clixon_err(OE_UNIX, EINVAL, "x0 or x1 is NULL"); + goto done; + } yt = xml_spec(x0); /* can be null */ xml_spec_set(x1, yt); /* Copy prefix*/ diff --git a/test/test_nacm_datanode_read.sh b/test/test_nacm_datanode_read.sh index fe0ab207..a8b650db 100755 --- a/test/test_nacm_datanode_read.sh +++ b/test/test_nacm_datanode_read.sh @@ -60,6 +60,7 @@ module nacm-example{ prefix ex2; } container table{ + presence "Tests rely on empty table"; container parameters{ list parameter{ key name;