diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e4edfd5..6628dc52 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,9 @@ Expected: January 2025 ### Features -* C-API: New no-copy `xmldb_get_cache` function for performance +* Performance optimization + * New no-copy `xmldb_get_cache` function for performance + * Optimized duplicate detection * New: CLI generic pipe callbacks * Add scripts in `CLICON_CLI_PIPE_DIR` * New: [feature request: support xpath functions for strings](https://github.com/clicon/clixon/issues/556) diff --git a/apps/backend/backend_client.c b/apps/backend/backend_client.c index 13f88d66..6c246d7d 100644 --- a/apps/backend/backend_client.c +++ b/apps/backend/backend_client.c @@ -667,13 +667,20 @@ from_client_edit_config(clixon_handle h, goto done; /* Disable duplicate check in NETCONF messages. */ if (clicon_option_bool(h, "CLICON_NETCONF_DUPLICATE_ALLOW")){ - if ((ret = xml_duplicate_remove_recurse(xc, &xret)) < 0) + if (xml_duplicate_remove_recurse(xc) < 0) goto done; } - else if ((ret = xml_yang_validate_unique_recurse(xc, &xret)) < 0) - goto done; + else { + if ((ret = xml_yang_validate_unique_recurse(xc, &xret)) < 0) + goto done; + if (ret == 0){ + if (clixon_xml2cbuf(cbret, xret, 0, 0, NULL, -1, 0) < 0) + goto done; + goto ok; + } + } /* xmldb_put (difflist handling) requires list keys */ - if (ret == 1 && (ret = xml_yang_validate_list_key_only(xc, &xret)) < 0) + if ((ret = xml_yang_validate_list_key_only(xc, &xret)) < 0) goto done; if (ret == 0){ if (clixon_xml2cbuf(cbret, xret, 0, 0, NULL, -1, 0) < 0) diff --git a/lib/clixon/clixon_validate_minmax.h b/lib/clixon/clixon_validate_minmax.h index 0c6f0f61..7b2c2bdd 100644 --- a/lib/clixon/clixon_validate_minmax.h +++ b/lib/clixon/clixon_validate_minmax.h @@ -47,6 +47,6 @@ int xml_yang_validate_minmax(cxobj *xt, int presence, cxobj **xret); int xml_yang_validate_minmax_recurse(cxobj *xt, cxobj **xret); int xml_yang_validate_unique(cxobj *xt, cxobj **xret); int xml_yang_validate_unique_recurse(cxobj *xt, cxobj **xret); -int xml_duplicate_remove_recurse(cxobj *xt, cxobj **xret); +int xml_duplicate_remove_recurse(cxobj *xt); #endif /* _CLIXON_VALIDATE_MINMAX_H_ */ diff --git a/lib/clixon/clixon_xml.h b/lib/clixon/clixon_xml.h index 8b1e18b3..876494c7 100644 --- a/lib/clixon/clixon_xml.h +++ b/lib/clixon/clixon_xml.h @@ -250,6 +250,7 @@ cxobj *xml_child_i(cxobj *xn, int i); cxobj *xml_child_i_type(cxobj *xn, int i, enum cxobj_type type); cxobj *xml_child_i_set(cxobj *xt, int i, cxobj *xc); int xml_child_order(cxobj *xn, cxobj *xc); +int xml_vector_decrement(cxobj *x, int nr); cxobj *xml_child_each(cxobj *xparent, cxobj *xprev, enum cxobj_type type); cxobj *xml_child_each_attr(cxobj *xparent, cxobj *xprev); int xml_child_insert_pos(cxobj *x, cxobj *xc, int pos); diff --git a/lib/src/clixon_validate_minmax.c b/lib/src/clixon_validate_minmax.c index 04a68655..57c898a3 100644 --- a/lib/src/clixon_validate_minmax.c +++ b/lib/src/clixon_validate_minmax.c @@ -46,6 +46,7 @@ #include #include #include +#include #include #include #include @@ -79,6 +80,19 @@ #include "clixon_xml_bind.h" #include "clixon_validate_minmax.h" +/* + * Local types + */ + +/*! List used to order values, object and strings for leaf-lists, cvk:s for lists + * consider union + */ +struct vec_order { + cxobj *vo_xml; + char **vo_strvec; + size_t vo_slen; /* length of vo_strvec (is actually global to vector) */ +}; + /*! New element last in list, check if already exists if so return -1 * * @param[in] vec Vector of existing entries (new is last) @@ -112,7 +126,7 @@ unique_search_xpath(cxobj *x, xi = xvec[i]; if ((bi = xml_body(xi)) == NULL) break; - /* Check if bi is duplicate? + /* Check if bi is duplicate? * XXX: sort svec? */ for (s=0; s<(*slen); s++){ @@ -137,7 +151,7 @@ unique_search_xpath(cxobj *x, goto done; } -/*! New element last in list, return error if already exists +/*! New element last in list, return error if already exists * * @param[in] vec Vector of existing entries (new is last) * @param[in] i1 The new entry is placed at vec[i1] @@ -253,18 +267,19 @@ check_unique_list_direct(cxobj *x, /* No keys: no checks necessary */ goto ok; } - /* x need not be child 0, which could make the vector larger than necessary */ + /* Vector of key values, k00,k01,..,k0n,k10,k11,.. + * Ie, if nr of keys is n, and nr of children is m, then length is n*m + * x need not be child 0, which could make the vector larger than necessary */ if ((vec = calloc(clen*xml_child_nr(xt), sizeof(char*))) == NULL){ clixon_err(OE_UNIX, errno, "calloc"); goto done; } + /* Vector of xml objects (children), not really necessary, is a direct copy of x_childvec */ if ((xvec = calloc(xml_child_nr(xt), sizeof(cxobj*))) == NULL){ clixon_err(OE_UNIX, errno, "calloc"); goto done; } - /* A vector is built with key-values, for each iteration check "backward" in the vector - * for duplicates - */ + /* Loop over children, then over each key, then search "backwards" */ i = 0; /* x element index */ do { xvec[i] = x; @@ -465,7 +480,7 @@ check_minmax(cxobj *xp, * @retval 1 Validation OK * @retval 0 Validation failed (xret set) * @retval -1 Error - * @note recurse for non-presence container + * @note recurse for non-presence container */ static int check_empty_list_minmax(cxobj *xt, @@ -542,9 +557,9 @@ xml_yang_minmax_new_list(cxobj *x, if (yang_keyword_get(yu) != Y_UNIQUE) continue; /* Here is a list w unique constraints identified by: - * its first element x, its yang spec y, its parent xt, and + * its first element x, its yang spec y, its parent xt, and * a unique yang spec yu, - * Two cases: + * Two cases: * 1) multiple direct children (no prefixes), eg "a b" * 2) single xpath with canonical prefixes, eg "/ex:a/ex:b" */ @@ -625,7 +640,7 @@ xml_yang_minmax_new_leaf_list(cxobj *x0, /*! Perform gap analysis in a child-vector interval [ye,y] * * Gap analysis here meaning if there is a list x with min-element constraint but there are no - * x elements in an interval of the children of xt. + * x elements in an interval of the children of xt. * For example, assume the yang of xt is yt and is defined as: * yt { * list a; @@ -634,8 +649,8 @@ xml_yang_minmax_new_leaf_list(cxobj *x0, * } * Further assume that xt is: * - * Then a call to this function could be ye=a, y=b. - * By iterating over the children of yt in the interval [a,b] it will find "x" with a min-element + * Then a call to this function could be ye=a, y=b. + * By iterating over the children of yt in the interval [a,b] it will find "x" with a min-element * constraint. */ static int @@ -702,13 +717,13 @@ xml_yang_minmax_gap_analysis(cxobj *xt, * x -> bc * x -> ab * - * Min-max constraints: + * Min-max constraints: * Find upper and lower bound of existing lists and report violations * Somewhat tricky to find violation of min-elements of empty * lists, but this is done by a "gap-detection" mechanism, which detects - * gaps in the xml nodes given the ancestor Yang structure. + * gaps in the xml nodes given the ancestor Yang structure. * But no gap analysis is done if the yang spec of the top-level xml is unknown - * Example: + * Example: * Yang structure: y1, y2, y3, * XML structure: [x1, x1], [x3, x3] where [x2] list is missing * @note min-element constraints on empty lists are not detected on top-level. @@ -716,8 +731,8 @@ xml_yang_minmax_gap_analysis(cxobj *xt, * XML node. This may not be a large problem since it would mean empty configs * are not allowed. * RFC 7950 7.7.5: regarding min-max elements check - * The behavior of the constraint depends on the type of the - * leaf-list's or list's closest ancestor node in the schema tree + * The behavior of the constraint depends on the type of the + * leaf-list's or list's closest ancestor node in the schema tree * that is not a non-presence container (see Section 7.5.1): * o If no such ancestor exists in the schema tree, the constraint * is enforced. @@ -733,9 +748,9 @@ xml_yang_minmax_gap_analysis(cxobj *xt, * 3 null list neq gap analysis, new: list key check * 4 nolist list neq gap analysis, new: list key check * 5 nolist nolist eq error - * 6 nolist nolist neq gap analysis; nopresence-check; - * 7 null nolist neq gap analysis; nopresence-check; - * 8 list nolist neq gap analysis; yprev: check-minmax; nopresence-check; + * 6 nolist nolist neq gap analysis; nopresence-check; + * 7 null nolist neq gap analysis; nopresence-check; + * 8 list nolist neq gap analysis; yprev: check-minmax; nopresence-check; * @param[in] xt XML parent (may have lists w unique constraints as child) * @param[in] presence Set if called in a recursive loop (the caller will recurse anyway), * otherwise non-presence containers will be traversed @@ -1035,118 +1050,253 @@ xml_yang_validate_unique_recurse(cxobj *xt, goto done; } -/*! YANG unique check and remove duplicates, keep last - * - * Assume xt:s children are sorted and yang populated. - * @param[in] xt XML parent (may have lists w unique constraints as child) - * @param[out] xret Error XML tree. Free with xml_free after use - * @retval 2 Locally abort this subtree, continue with others - * @retval 1 Abort, dont continue with others, return 1 to end user - * @retval 0 OK, continue - * @retval -1 Error, aborted at first error encounter, return -1 to end user - * @see xml_yang_validate_minmax which include these unique tests +/*----------- New linear vector code -----------------*/ + +static int +vec_free(struct vec_order *vec, + size_t vlen) +{ + int v; + + for (v=0; vvo_slen; i++){ + assert(v1->vo_strvec[i]); + assert(v2->vo_strvec[i]); + if ((eq = strcmp(v1->vo_strvec[i], v2->vo_strvec[i])) != 0) + break; + } + if (eq != 0) + return eq; + i1 = xml_enumerate_get(v1->vo_xml); + i2 = xml_enumerate_get(v2->vo_xml); + if (i1 > i2) + return 1; + else if (i1 < i2) + return -1; + else + return 0; +} + +/*! Remove duplicates list + */ +static int +remove_duplicates_list(struct vec_order *vec, + size_t vlen, + int *nr) +{ + int retval = -1; + int v; + int i; + + if (nr) + *nr = 0; + for (v=1; v0 && slen0 != clen){ /* Sanity check */ + clixon_err(OE_YANG, 0, "List key vector mismatch %lu != %lu", slen0, clen); + goto done; + } + /* see check_unique_list_direct */ + if ((vec = realloc(vec, (vlen+1)*sizeof(*vec))) == NULL){ + clixon_err(OE_UNIX, errno, "cvec_new"); + goto done; + } + vec[vlen].vo_slen = clen; + vec[vlen].vo_xml = x; + if ((vec[vlen].vo_strvec = calloc(vec[vlen].vo_slen , sizeof(char*))) == NULL){ + clixon_err(OE_UNIX, errno, "calloc"); + goto done; + } + cvi = NULL; + v = 0; + while ((cvi = cvec_each(cvk, cvi)) != NULL){ + if ((str = cv_string_get(cvi)) == NULL) + break; + if ((xi = xml_find(x, str)) == NULL) + break; + if ((b = xml_body(xi)) == NULL) + vec[vlen].vo_strvec[v++] = ""; + else + vec[vlen].vo_strvec[v++] = b; + } + if (cvi != NULL){ /* No key or null: revert and skip */ + free(vec[vlen].vo_strvec); + memset(&vec[vlen], 0, sizeof(*vec)); + vec[vlen].vo_strvec = NULL; + } + else{ + slen0 = clen; + vlen++; + } + break; + case Y_LEAF_LIST: + if (vec>0 && slen0 != 1){ /* Sanity check */ + clixon_err(OE_YANG, 0, "Leaf-list key vector mismatch %lu != 1", slen0); + goto done; + } + if ((vec = realloc(vec, (vlen+1)*sizeof(struct vec_order))) == NULL){ + clixon_err(OE_UNIX, errno, "cvec_new"); + goto done; + } + vec[vlen].vo_xml = x; + vec[vlen].vo_slen = 1; + if ((vec[vlen].vo_strvec = calloc(vec[vlen].vo_slen, sizeof(char*))) == NULL){ + clixon_err(OE_UNIX, errno, "calloc"); + goto done; + } + b = xml_body(x); + vec[vlen].vo_strvec[0] = b; + slen0 = 1; + vlen++; + break; + default: + break; + } + y0 = y; + } + if (y0 && vec != NULL){ + if (vec_order_analyze(y0, vec, vlen, NULL) < 0) + goto done; + if (vec_free(vec, vlen) < 0) + goto done; + vec = NULL; + vlen = 0; + } + retval = 0; + done: + if (vec) + free(vec); + return retval; } /*! Recursive YANG unique check and remove duplicates, keep last * * @param[in] xt XML parent (may have lists w unique constraints as child) - * @param[out] xret Error XML tree. Free with xml_free after use - * @retval 1 Validation OK - * @retval 0 Validation failed (xret set) + * @retval 0 Validation OK * @retval -1 Error - * @see xml_yang_validate_unique_recurse + * @see xml_yang_validate_unique_recurse This function destructively removes */ int -xml_duplicate_remove_recurse(cxobj *xt, - cxobj **xret) +xml_duplicate_remove_recurse(cxobj *xt) { - int retval = -1; - int ret; + int retval = -1; + cxobj *x; - if ((ret = xml_apply0(xt, CX_ELMNT, xml_duplicate_remove, xret)) < 0) + if (xml_duplicate_remove(xt) < 0) goto done; - if (ret == 1) - goto fail; - retval = 1; + x = NULL; + while ((x = xml_child_each(xt, x, CX_ELMNT)) != NULL) { + if (xml_duplicate_remove_recurse(x) < 0) + goto done; + } + retval = 0; done: return retval; - fail: - retval = 0; - goto done; } diff --git a/lib/src/clixon_xml.c b/lib/src/clixon_xml.c index 0447f05a..908a6211 100644 --- a/lib/src/clixon_xml.c +++ b/lib/src/clixon_xml.c @@ -909,6 +909,16 @@ xml_child_order(cxobj *xp, return -1; } +/*! Advanced function to decrement _x_vector_i if objects have been removed + */ +int +xml_vector_decrement(cxobj *x, + int nr) +{ + x->_x_vector_i -= nr; + return 0; +} + /*! Iterator over xml children objects * * @param[in] xparent xml tree node whose children should be iterated diff --git a/test/test_netconf_duplicate.sh b/test/test_netconf_duplicate.sh index c8bada6b..9c3e0687 100755 --- a/test/test_netconf_duplicate.sh +++ b/test/test_netconf_duplicate.sh @@ -58,6 +58,10 @@ module unique{ leaf-list b{ type string; } + leaf-list buser{ + ordered-by user; + type string; + } } } EOF @@ -98,7 +102,7 @@ expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" " " "" "applicationoperation-faileddata-not-uniqueerror/rpc/edit-config/config/c/server[name=\"one\"]/name" -new "Add list with duplicate" +new "Add list with duplicate 2" expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "replace one @@ -142,9 +146,8 @@ if [ $BE -ne 0 ]; then fi # Check CLICON_NETCONF_DUPLICATE_ALLOW - if [ $BE -ne 0 ]; then - new "start backend -s init -f $cfg" + new "start backend -s init -f $cfg -o CLICON_NETCONF_DUPLICATE_ALLOW=true" # start new backend start_backend -s init -f $cfg -o CLICON_NETCONF_DUPLICATE_ALLOW=true fi @@ -246,16 +249,82 @@ expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" " new "netconf discard-changes" expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "" -new "Add leaf-list with duplicate" +new "leaf-list with dups" expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "replace - aaa + ccc aaa bbb + aaa " "" "" -new "Check leaf-list no duplicates" -expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "aaabbb" +new "Check leaf-list with dups" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "aaabbbccc" +new "leaf-list with dups ordered-by user" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "replace + CCC + AAA + BBB + AAA + AAA +" "" "" + +new "Check" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "CCCBBBAAA" + +new "list/leaf-list mix" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "replace + ccc + CCC + + bbb + foo + + aaa + AAA + + aaa + foo + + bbb + BBB + + bbb + foo + + aaa + AAA +" "" "" + +new "Check mix" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "aaafoobbbfooaaabbbcccCCCBBBAAA" + +new "Mix with empty" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "replace + ccc + CCC + + + foo + + + AAA + + aaa + foo + + bbb + BBB + + + foo + + + AAA +" "" "" + +new "Check mix w empty" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "aaafoofoobbbcccCCCBBBAAA" if [ $BE -ne 0 ]; then new "Kill backend"