From 02d725b2c02f3e7873bafb0b190b8fe5b029a5a4 Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Sun, 3 Feb 2019 16:19:33 +0100 Subject: [PATCH] * xml_cmp() respects 'ordered-by user' for state nodes, which violates RFC 7950 [https://github.com/clicon/clixon/issues/63. (Thanks JDL) --- CHANGELOG.md | 1 + example/example_backend.c | 2 + lib/clixon/clixon_xml_sort.h | 1 - lib/src/clixon_xml_sort.c | 27 ++++++++++--- test/test_feature.sh | 2 +- test/test_nacm_ext.sh | 2 +- test/test_nacm_module_read.sh | 6 +-- test/test_netconf.sh | 2 +- test/test_order.sh | 76 ++++++++++++++++++++--------------- test/test_restconf.sh | 19 ++++----- 10 files changed, 85 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index adc119052..80474fd1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -142,6 +142,7 @@ * JSON conversion problems [https://github.com/clicon/clixon/issues/66] * CDATA sections stripped from XML when converted to JSON * Restconf returns error when RPC generates "ok" reply [https://github.com/clicon/clixon/issues/69] diff --git a/example/example_backend.c b/example/example_backend.c index 2c366a146..2e074064b 100644 --- a/example/example_backend.c +++ b/example/example_backend.c @@ -201,6 +201,8 @@ example_statedata(clicon_handle h, */ if (xml_parse_string("" "42" + "41" + "43" /* should not be ordered */ "", NULL, &xstate) < 0) goto done; retval = 0; diff --git a/lib/clixon/clixon_xml_sort.h b/lib/clixon/clixon_xml_sort.h index 8b1e6f825..d38b91cf5 100644 --- a/lib/clixon/clixon_xml_sort.h +++ b/lib/clixon/clixon_xml_sort.h @@ -40,7 +40,6 @@ * Prototypes */ int xml_child_spec(cxobj *x, cxobj *xp, yang_spec *yspec, yang_stmt **yp); -int xml_cmp(const void* arg1, const void* arg2); int xml_sort(cxobj *x0, void *arg); cxobj *xml_search(cxobj *x, char *name, int yangi, enum rfc_6020 keyword, int keynr, char **keyvec, char **keyval); int xml_insert_pos(cxobj *x0, char *name, int yangi, enum rfc_6020 keyword, diff --git a/lib/src/clixon_xml_sort.c b/lib/src/clixon_xml_sort.c index fb5e412ba..162735f71 100644 --- a/lib/src/clixon_xml_sort.c +++ b/lib/src/clixon_xml_sort.c @@ -134,7 +134,7 @@ xml_child_spec(cxobj *x, * @see xml_cmp1 Similar, but for one object * @note empty value/NULL is smallest value */ -int +static int xml_cmp(const void* arg1, const void* arg2) { @@ -162,11 +162,13 @@ xml_cmp(const void* arg1, if ((equal = yi1-yi2) != 0) return equal; } - /* Now y1=y2, same Yang spec, can only be list or leaf-list, - * sort according to key + /* Now y1==y2, same Yang spec, can only be list or leaf-list, + * But first check exceptions, eg config false or ordered-by user + * otherwuse sort according to key */ - if (yang_find((yang_node*)y1, Y_ORDERED_BY, "user") != NULL) - return 0; /* Ordered by user: maintain existing order */ + if (yang_config(y1)==0 || + yang_find((yang_node*)y1, Y_ORDERED_BY, "user") != NULL) + return 0; /* Ordered by user or state data : maintain existing order */ switch (y1->ys_keyword){ case Y_LEAF_LIST: /* Match with name and value */ if ((b1 = xml_body(x1)) == NULL) @@ -262,11 +264,20 @@ xml_cmp1(cxobj *x, * Assume populated by yang spec. * @param[in] x0 XML node * @param[in] arg Dummy so it can be called by xml_apply() + * @retval -1 Error, aborted at first error encounter + * @retval 0 OK, all nodes traversed (subparts may have been skipped) + * @retval 1 OK, aborted on first fn returned 1 + * @see xml_apply - typically called by recursive apply function */ int xml_sort(cxobj *x, void *arg) { + yang_stmt *ys; + + /* Abort sort if non-config (=state) data */ + if ((ys = xml_spec(x)) != 0 && yang_config(ys)==0) + return 1; qsort(xml_childvec_get(x), xml_child_nr(x), sizeof(cxobj *), xml_cmp); return 0; } @@ -543,7 +554,13 @@ xml_sort_verify(cxobj *x0, int retval = -1; cxobj *x = NULL; cxobj *xprev = NULL; + yang_stmt *ys; + /* Abort sort if non-config (=state) data */ + if ((ys = xml_spec(x0)) != 0 && yang_config(ys)==0){ + retval = 1; + goto done; + } while ((x = xml_child_each(x0, x, -1)) != NULL) { if (xprev != NULL){ /* Check xprev <= x */ if (xml_cmp(&xprev, &x) > 0) diff --git a/test/test_feature.sh b/test/test_feature.sh index 3dd1e3d40..dabc8a3cd 100755 --- a/test/test_feature.sh +++ b/test/test_feature.sh @@ -145,7 +145,7 @@ fi # Note order of features in ietf-netconf yang is alphabetically: candidate, startup, validate, xpath new "netconf module ietf-netconf" -expect="ietf-netconf2011-06-01urn:ietf:params:xml:ns:netconf:base:1.0candidatestartupvalidatexpathimplement" +expect="ietf-netconf2011-06-01urn:ietf:params:xml:ns:netconf:base:1.0candidatevalidatestartupxpathimplement" match=`echo "$ret" | grep -GZo "$expect"` if [ -z "$match" ]; then err "$expect" "$ret" diff --git a/test/test_nacm_ext.sh b/test/test_nacm_ext.sh index abc5f4104..f9ed5fe10 100755 --- a/test/test_nacm_ext.sh +++ b/test/test_nacm_ext.sh @@ -152,7 +152,7 @@ sudo su -c "$clixon_restconf -f $cfg -D $DBG -- -a" -s /bin/sh www-data & sleep $RCWAIT new "auth get" -expecteq "$(curl -u andy:bar -sS -X GET http://localhost/restconf/data/clixon-example:state)" '{"clixon-example:state": {"op": "42"}} +expecteq "$(curl -u andy:bar -sS -X GET http://localhost/restconf/data/clixon-example:state)" '{"clixon-example:state": {"op": ["42","41","43"]}} ' new "Set x to 0" diff --git a/test/test_nacm_module_read.sh b/test/test_nacm_module_read.sh index 252bfd31b..47d70163a 100755 --- a/test/test_nacm_module_read.sh +++ b/test/test_nacm_module_read.sh @@ -175,7 +175,7 @@ expecteq "$(curl -u andy:bar -sS -X GET http://localhost/restconf/data/nacm-exam ' new "admin read state OK" -expecteq "$(curl -u andy:bar -sS -X GET http://localhost/restconf/data/clixon-example:state)" '{"clixon-example:state": {"op": "42"}} +expecteq "$(curl -u andy:bar -sS -X GET http://localhost/restconf/data/clixon-example:state)" '{"clixon-example:state": {"op": ["42","41","43"]}} ' new "admin read top ok (all)" @@ -204,11 +204,11 @@ expecteq "$(curl -u wilma:bar -sS -X GET http://localhost/restconf/data/nacm-exa ' new "limit read state OK" -expecteq "$(curl -u wilma:bar -sS -X GET http://localhost/restconf/data/clixon-example:state)" '{"clixon-example:state": {"op": "42"}} +expecteq "$(curl -u wilma:bar -sS -X GET http://localhost/restconf/data/clixon-example:state)" '{"clixon-example:state": {"op": ["42","41","43"]}} ' new "limit read top ok (part)" -expecteq "$(curl -u wilma:bar -sS -X GET http://localhost/restconf/data)" '{"data": {"clixon-example:translate": [{"k": "key42","value": "val42"},{ "k": "key43","value": "val43"}],"clixon-example:state": {"op": "42"}}} +expecteq "$(curl -u wilma:bar -sS -X GET http://localhost/restconf/data)" '{"data": {"clixon-example:translate": [{"k": "key42","value": "val42"},{ "k": "key43","value": "val43"}],"clixon-example:state": {"op": ["42","41","43"]}}} ' #user:guest diff --git a/test/test_netconf.sh b/test/test_netconf.sh index eb3c0ed67..d1f76ab07 100755 --- a/test/test_netconf.sh +++ b/test/test_netconf.sh @@ -155,7 +155,7 @@ new "netconf edit state operation should fail" expecteof "$clixon_netconf -qf $cfg" 0 '42]]>]]>' "^protocolinvalid-value" new "netconf get state operation" -expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" '^42]]>]]>$' +expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" '^424143]]>]]>$' new "netconf lock/unlock" expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>]]>]]>" "^]]>]]>]]>]]>$" diff --git a/test/test_order.sh b/test/test_order.sh index a1f646db8..1f6d0c477 100755 --- a/test/test_order.sh +++ b/test/test_order.sh @@ -10,6 +10,7 @@ APPNAME=example . ./lib.sh cfg=$dir/conf_yang.xml fyang=$dir/order.yang +tmp=$dir/tmp.x # For memcheck # clixon_netconf="valgrind --leak-check=full --show-leak-kinds=all clixon_netconf" @@ -30,6 +31,7 @@ cat < $cfg /usr/local/lib/$APPNAME/cli $APPNAME /usr/local/var/$APPNAME/$APPNAME.sock + /usr/local/lib/example/backend /usr/local/var/$APPNAME/$APPNAME.pidfile 1 $dbdir @@ -39,10 +41,13 @@ cat < $cfg EOF cat < $fyang -module example{ +module order-example{ yang-version 1.1; - namespace "urn:example:clixon"; + namespace "urn:example:order"; prefix ex; + import clixon-example { /* for state callback */ + prefix ex; + } container c{ leaf d{ type string; @@ -86,24 +91,24 @@ rm -f $dbdir/candidate_db # alt cat < $dbdir/running_db - d - d - dbar - dbar - b - b - hej - c - c - abar - abar - hopp - a - a - cbar - cbar - bbar - bbar + d + d + dbar + dbar + b + b + hej + c + c + abar + abar + hopp + a + a + cbar + cbar + bbar + bbar EOF @@ -122,21 +127,28 @@ if [ $BE -ne 0 ]; then fi fi +# STATE (should not be ordered) +new "state data (should be unordered: 42,41,43)" +cat < $tmp +]]>]]> +EOF +expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 "$(cat $tmp)" '424143]]>]]>' + # Check as file new "verify running from start, should be: c,l,y0,y1,y2,y3; y1 and y3 sorted. Note this fails if CLICON_XML_SORT set to false" -expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 ']]>]]>' '^hejhoppdbcaabcddbarabarcbarbbarabarbbarcbardbar]]>]]>$' +expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 ']]>]]>' '^hejhoppdbcaabcddbarabarcbarbbarabarbbarcbardbarloex:loopbacktrue]]>]]>$' new "get each ordered-by user leaf-list" -expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 "]]>]]>" '^abar]]>]]>$' +expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 "]]>]]>" '^abar]]>]]>$' new "get each ordered-by user leaf-list" -expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 "]]>]]>" '^abar]]>]]>$' +expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 "]]>]]>" '^abar]]>]]>$' new "get each ordered-by user leaf-list" -expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 "]]>]]>" '^bbar]]>]]>$' +expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 "]]>]]>" '^bbar]]>]]>$' new "get each ordered-by user leaf-list" -expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 "]]>]]>" '^bbar]]>]]>$' +expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 "]]>]]>" '^bbar]]>]]>$' new "delete candidate" expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 'none]]>]]>' "^]]>]]>$" @@ -144,33 +156,33 @@ expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 'cb]]>]]>' "^]]>]]>$" +expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 'cb]]>]]>' "^]]>]]>$" new "add one entry (a) to leaf-list user order" -expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 'a]]>]]>' "^]]>]]>$" +expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 'a]]>]]>' "^]]>]]>$" new "netconf commit" expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 "]]>]]>" "^]]>]]>$" new "add one entry (0) to leaf-list user order after commit" -expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 '0]]>]]>' "^]]>]]>$" +expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 '0]]>]]>' "^]]>]]>$" new "netconf commit" expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 "]]>]]>" "^]]>]]>$" new "verify leaf-list user order in running (as entered: c,b,a,0)" -expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 ']]>]]>' '^cba0]]>]]>$' +expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 ']]>]]>' '^cba0]]>]]>$' # LISTS new "add two entries to list user order" -expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 'cbarbfoo]]>]]>' "^]]>]]>$" +expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 'cbarbfoo]]>]]>' "^]]>]]>$" new "add one entry to list user order" -expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 'afie]]>]]>' "^]]>]]>$" +expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 'afie]]>]]>' "^]]>]]>$" new "verify list user order (as entered)" -expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 ']]>]]>' '^cbarbfooafie]]>]]>$' +expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 ']]>]]>' '^cbarbfooafie]]>]]>$' if [ $BE -eq 0 ]; then exit # BE diff --git a/test/test_restconf.sh b/test/test_restconf.sh index 78f074a8c..14c3dbc85 100755 --- a/test/test_restconf.sh +++ b/test/test_restconf.sh @@ -31,7 +31,7 @@ cat < $cfg EOF # This is a fixed 'state' implemented in routing_backend. It is assumed to be always there -state='{"clixon-example:state": {"op": "42"}}' +state='{"clixon-example:state": {"op": ["42","41","43"]}} ' new "test params: -f $cfg" if [ $BE -ne 0 ]; then @@ -113,11 +113,11 @@ new "restconf empty rpc with extra args (should fail)" expecteq "$(curl -s -X POST -d {\"clixon-example:input\":{\"extra\":null}} http://localhost/restconf/operations/clixon-example:empty)" '{"ietf-restconf:errors" : {"error": {"error-type": "application","error-tag": "unknown-element","error-info": {"bad-element": "extra"},"error-severity": "error"}}} ' new "restconf get empty config + state json" -expecteq "$(curl -sSG http://localhost/restconf/data/clixon-example:state)" '{"clixon-example:state": {"op": "42"}} +expecteq "$(curl -sSG http://localhost/restconf/data/clixon-example:state)" '{"clixon-example:state": {"op": ["42","41","43"]}} ' new "restconf get empty config + state json + module" -expecteq "$(curl -sSG http://localhost/restconf/data/clixon-example:state)" '{"clixon-example:state": {"op": "42"}} +expecteq "$(curl -sSG http://localhost/restconf/data/clixon-example:state)" '{"clixon-example:state": {"op": ["42","41","43"]}} ' new "restconf get empty config + state json with wrong module name" @@ -125,14 +125,14 @@ expecteq "$(curl -sSG http://localhost/restconf/data/badmodule:state)" '{"ietf-r new "restconf get empty config + state xml" ret=$(curl -s -H "Accept: application/yang-data+xml" -G http://localhost/restconf/data/clixon-example:state) -expect='42' +expect='424143' match=`echo $ret | grep -EZo "$expect"` if [ -z "$match" ]; then err "$expect" "$ret" fi new "restconf get data/ json" -expecteq "$(curl -s -G http://localhost/restconf/data/clixon-example:state/op=42)" '{"clixon-example:op": "42"} +expecteq "$(curl -s -G http://localhost/restconf/data/clixon-example:state/op=42)" '{"clixon-example:op": ["42","41","43"]} ' new "restconf get state operation eth0 xml" @@ -145,7 +145,7 @@ if [ -z "$match" ]; then fi new "restconf get state operation eth0 type json" -expecteq "$(curl -s -G http://localhost/restconf/data/clixon-example:state/op=42)" '{"clixon-example:op": "42"} +expecteq "$(curl -s -G http://localhost/restconf/data/clixon-example:state/op=42)" '{"clixon-example:op": ["42","41","43"]} ' new "restconf get state operation eth0 type xml" @@ -158,7 +158,7 @@ if [ -z "$match" ]; then fi new "restconf GET datastore" -expecteq "$(curl -s -X GET http://localhost/restconf/data/clixon-example:state)" '{"clixon-example:state": {"op": "42"}} +expecteq "$(curl -s -X GET http://localhost/restconf/data/clixon-example:state)" '{"clixon-example:state": {"op": ["42","41","43"]}} ' # Exact match @@ -178,7 +178,8 @@ new "restconf delete interfaces" expecteq $(curl -s -X DELETE http://localhost/restconf/data/ietf-interfaces:interfaces) "" new "restconf Check empty config" -expectfn "curl -sG http://localhost/restconf/data/clixon-example:state" 0 "$state" +expectfn "curl -sG http://localhost/restconf/data/clixon-example:state" 0 "$state + " # XXX: gives # @@ -192,7 +193,7 @@ expecteq "$(curl -s -G http://localhost/restconf/data/ietf-interfaces:interfaces ' new "restconf Check eth/0/0 added state" -expecteq "$(curl -s -G http://localhost/restconf/data/clixon-example:state)" '{"clixon-example:state": {"op": "42"}} +expecteq "$(curl -s -G http://localhost/restconf/data/clixon-example:state)" '{"clixon-example:state": {"op": ["42","41","43"]}} ' new "restconf Re-post eth/0/0 which should generate error"