From feec3a21d9748f2addc77e8bd6dd3f95676ffba5 Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Sun, 17 Apr 2022 10:34:55 +0200 Subject: [PATCH] YANG key list and unique fixes * Fixed: YANG key list check bad performance * List key check did unique "xpath" lookup instead of direct child traverse * Fixed: YANG unique single schema-nodeid required "canonical" namespace * E.g., `a/b` did not work even if there was default namespace in XML * Memory leak in xpath parser --- CHANGELOG.md | 10 ++- lib/src/clixon_validate.c | 148 +++++++++++++++++++++-------------- lib/src/clixon_xpath_parse.y | 3 +- 3 files changed, 97 insertions(+), 64 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8dab8fe4..1b259b90 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -86,6 +86,13 @@ Users may have to change how they access the system ### Corrected Bugs +* Fixed: YANG key list check bad performance + * List key check did unique "xpath" lookup instead of direct child traverse +* Fixed: YANG unique single schema-nodeid required "canonical" namespace + * E.g., `a/b` did not work even if there was default namespace in XML +* Disabled xpath optimization for hierarchical list + * When `XPATH_LIST_OPTIMIZE` is set, patterns like `y[k='3']` is optimized + * But hierarchical lists should not be, ie when `a/y[k='3']` and `a` is a list * Fixed: Removed warning at startup: `No YANG spec for module-set` * Fixed: HTTP/1 multiple write requests in single session appended data between writes, eg PUT+PUT. * Fixed: [Broken pipe error seen in client (cli) when backend restarts and CLICON_SOCK is recreated](https://github.com/clicon/clixon/issues/312) @@ -168,9 +175,6 @@ Users may have to change how they access the system ### Corrected Bugs -* Disabled xpath optimization for hierarchical list - * When `XPATH_LIST_OPTIMIZE` is set, patterns like `y[k='3']` is optimized - * But hierarchical lists should not be, ie when `a/y[k='3']` and `a` is a list * Fixed: [Validate error when appending module B grouping to module A item use augment statement #308](https://github.com/clicon/clixon/issues/308) * Fixed: [Restconf PATCH method request failed on item defined by submodule #306](https://github.com/clicon/clixon/issues/306) * Fixed: [restconf GET json response does not encode top level node with namespace as per rfc #303](https://github.com/clicon/clixon/issues/303) diff --git a/lib/src/clixon_validate.c b/lib/src/clixon_validate.c index d0f942ec..4c3a2d6b 100644 --- a/lib/src/clixon_validate.c +++ b/lib/src/clixon_validate.c @@ -715,10 +715,11 @@ check_mandatory(cxobj *xt, * @retval -1 Error */ static int -unique_search_one(cxobj *x, - char *xpath, - char ***svec, - size_t *slen) +unique_search_xpath(cxobj *x, + char *xpath, + cvec *nsc, + char ***svec, + size_t *slen) { int retval = -1; cxobj **xvec = NULL; @@ -729,7 +730,7 @@ unique_search_one(cxobj *x, char *bi; /* Collect tuples */ - if (xpath_vec(x, NULL, "%s", &xvec, &xveclen, xpath) < 0) + if (xpath_vec(x, nsc, "%s", &xvec, &xveclen, xpath) < 0) goto done; for (i=0; i 1 */ while ((cvi = cvec_each(cvk, cvi)) != NULL){ /* RFC7950: Sec 7.8.3.1: entries that do not have value for all * referenced leafs are not taken into account */ - if ((xi = xml_find(x, cv_string_get(cvi))) == NULL) + str = cv_string_get(cvi); + if (index(str, '/') != NULL){ + clicon_err(OE_YANG, 0, "Multiple descendant nodes not allowed (w /)"); + goto done; + } + if ((xi = xml_find(x, str)) == NULL) break; if ((bi = xml_body(xi)) == NULL) break; @@ -905,45 +914,6 @@ check_unique_list_mult(cxobj *x, goto done; } -static int -check_unique_list_deep(cxobj *x, - cxobj *xt, - yang_stmt *y, - cvec *cvk, - cxobj **xret) -{ - int retval = -1; - cg_var *cvi; /* unique node name */ - char **svec = NULL; /* vector of search results */ - size_t slen = 0; - int ret; - - do { - cvi = NULL; - while ((cvi = cvec_each(cvk, cvi)) != NULL){ - /* Collect search results from one */ - if ((ret = unique_search_one(x, cv_string_get(cvi), &svec, &slen)) < 0) - goto done; - if (ret == 0){ - if (xret && netconf_data_not_unique_xml(xret, x, cvk) < 0) - goto done; - goto fail; - } - } /* cvi: unique arguments */ - x = xml_child_each(xt, x, CX_ELMNT); - } while (x && y == xml_spec(x)); /* stop if list ends, others may follow */ - // ok: - /* It would be possible to cache vec here as an optimization */ - retval = 1; - done: - if (svec) - free(svec); - return retval; - fail: - retval = 0; - goto done; -} - /*! Given a list with unique constraint, detect duplicates * @param[in] x The first element in the list (on return the last) * @param[in] xt The parent of x (a list) @@ -951,7 +921,7 @@ check_unique_list_deep(cxobj *x, * @param[in] yu A yang unique (Y_UNIQUE) for unique schema node ids or (Y_LIST) for list keys * @param[out] xret Error XML tree. Free with xml_free after use * @retval 1 Validation OK - * @retval 0 Validation failed (cbret set) + * @retval 0 Validation failed (xret set) * @retval -1 Error * Discussion: the RFC 7950 Sec 7.8.3: "constraints on valid list entries" * The arguments are "descendant schema node identifiers". A direct interpretation is that @@ -974,13 +944,68 @@ check_unique_list(cxobj *x, yang_stmt *yu, cxobj **xret) { - cvec *cvk; + int retval = -1; + cg_var *cvi; /* unique node name */ + char **svec = NULL; /* vector of search results */ + size_t slen = 0; + char *xpath0 = NULL; + char *xpath1 = NULL; + int ret; + cvec *cvk; + cvec *nsc0 = NULL; + cvec *nsc1 = NULL; + /* Check if multiple direct children */ cvk = yang_cvec_get(yu); - if (cvec_len(cvk) != 1) /* case 1: set of direct descendants */ - return check_unique_list_mult(x, xt, y, yu, cvk, xret); - else /* case 2: single deep schema node */ - return check_unique_list_deep(x, xt, y, cvk, xret); + if (cvec_len(cvk) > 1){ + retval = check_unique_list_direct(x, xt, y, yu, xret); + goto done; + } + cvi = cvec_i(cvk, 0); + if (cvi == NULL || (xpath0 = cv_string_get(cvi)) == NULL){ + clicon_err(OE_YANG, 0, "No descendant schemanode"); + goto done; + } + /* Check if direct schmeanode-id , ie not xpath */ + if (index(xpath0, '/') == NULL){ + retval = check_unique_list_direct(x, xt, y, yu, xret); + goto done; + } + /* Here proper xpath with at least one slash (can there be a descendant schemanodeid w/o slash?) */ + if (xml_nsctx_yang(yu, &nsc0) < 0) + goto done; + if ((ret = xpath2canonical(xpath0, nsc0, ys_spec(y), + &xpath1, &nsc1, NULL)) < 0) + goto done; + if (ret == 0) + goto fail; // XXX set xret + do { + /* Collect search results from one */ + if ((ret = unique_search_xpath(x, xpath1, nsc1, &svec, &slen)) < 0) + goto done; + if (ret == 0){ + if (xret && netconf_data_not_unique_xml(xret, x, cvk) < 0) + goto done; + goto fail; + } + x = xml_child_each(xt, x, CX_ELMNT); + } while (x && y == xml_spec(x)); /* stop if list ends, others may follow */ + // ok: + /* It would be possible to cache vec here as an optimization */ + retval = 1; + done: + if (nsc0) + cvec_free(nsc0); + if (nsc1) + cvec_free(nsc1); + if (xpath1) + free(xpath1); + if (svec) + free(svec); + return retval; + fail: + retval = 0; + goto done; } /*! Given a list, check if any min/max-elemants constraints apply @@ -1156,9 +1181,9 @@ check_list_unique_minmax(cxobj *xt, if (keyw != Y_LIST) continue; /* Here new (first element) of lists only - * First check unique keys + * First check unique keys direct children */ - if ((ret = check_unique_list(x, xt, y, y, xret)) < 0) + if ((ret = check_unique_list_direct(x, xt, y, y, xret)) < 0) goto done; if (ret == 0) goto fail; @@ -1171,6 +1196,9 @@ check_list_unique_minmax(cxobj *xt, /* Here is a list w unique constraints identified by: * its first element x, its yang spec y, its parent xt, and * a unique yang spec yu, + * Two cases: + * 1) multiple direct children (no prefixes), eg "a b" + * 2) single xpath with canonical prefixes, eg "/ex:a/ex:b" */ if ((ret = check_unique_list(x, xt, y, yu, xret)) < 0) goto done; diff --git a/lib/src/clixon_xpath_parse.y b/lib/src/clixon_xpath_parse.y index 2b76472a..d7f8af73 100644 --- a/lib/src/clixon_xpath_parse.y +++ b/lib/src/clixon_xpath_parse.y @@ -479,7 +479,8 @@ abbreviatedstep : '.' predicates { $$=xp_new(XP_STEP,A_SELF, NULL,NULL, NULL */ axisspec : NCNAME DOUBLECOLON { if (($$=xp_axisname_function(_XPY, $1)) < 0) YYERROR; - _PARSE_DEBUG2("axisspec-> AXISNAME(%s -> %d) ::", $1, $$); + free($1); + _PARSE_DEBUG2("axisspec-> AXISNAME(%s -> %d) ::", $1, $$); } | abbreviatedaxisspec { $$ = $1; }