diff --git a/CHANGELOG.md b/CHANGELOG.md index 3755a445..e0cf2259 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,6 +64,10 @@ Users may have to change how they access the system ### Minor features +* [Adding feature to top level container doesn't work](https://github.com/clicon/clixon/issues/322) + * Instead of removing YANG which is disabled by `if-feature`, replace it with an yang `anydata` node. + * This means XML specified by such YANG is ignored, and it is not an error to access it + * Note the similarity with `CLICON_YANG_UNKNOWN_ANYDATA` * [provide support for load config of cli format along with json and xml format as save config is supported for all 3 formats](https://github.com/clicon/clixon/issues/320) * [prevent clixon-restconf@2021-05-20.yang module from loading](https://github.com/clicon/clixon/issues/318) * Instead of always loading it, load it to datastore YANGs only if `CLICON_BACKEND_RESTCONF_PROCESS` is `true` diff --git a/lib/src/clixon_yang.c b/lib/src/clixon_yang.c index c6683383..6d5efac6 100644 --- a/lib/src/clixon_yang.c +++ b/lib/src/clixon_yang.c @@ -93,7 +93,7 @@ static int yang_search_index_extension(clicon_handle h, yang_stmt *yext, yang_st /* * Local variables */ -/* Mapping between yang keyword string <--> clicon constants +/* Mapping between yang keyword string <--> clixon constants * Here is also the place where doc on some types store variables (cv) */ static const map_str2int ykmap[] = { @@ -653,7 +653,7 @@ ys_free1(yang_stmt *ys, free(ys->ys_filename); if (self){ free(ys); - _stats_yang_nr++; + _stats_yang_nr--; } return 0; } @@ -723,6 +723,27 @@ ys_prune_self(yang_stmt *ys) return retval; } +/*! Free all yang children tree recursively + * @param[in] ys Yang node to its children recursively + */ +static int +ys_freechildren(yang_stmt *ys) +{ + int i; + yang_stmt *yc; + + for (i=0; iys_len; i++){ + if ((yc = ys->ys_stmt[i]) != NULL) + ys_free(yc); + } + ys->ys_len = 0; + if (ys->ys_stmt){ + free(ys->ys_stmt); + ys->ys_stmt = NULL; + } + return 0; +} + /*! Free a yang statement tree recursively * @param[in] ys Yang node to remove and all its children recursively * @note does not remove yang node from tree @@ -731,13 +752,7 @@ ys_prune_self(yang_stmt *ys) int ys_free(yang_stmt *ys) { - int i; - yang_stmt *yc; - - for (i=0; iys_len; i++){ - if ((yc = ys->ys_stmt[i]) != NULL) - ys_free(yc); - } + ys_freechildren(ys); ys_free1(ys, 1); return 0; } @@ -2925,7 +2940,7 @@ yang_features(clicon_handle h, int ret; i = 0; - while (iys_len){ /* Note, children may be removed */ + while (iys_len){ ys = yt->ys_stmt[i]; if (ys->ys_keyword == Y_IF_FEATURE){ if ((ret = yang_if_feature(h, ys)) < 0) @@ -2933,25 +2948,34 @@ yang_features(clicon_handle h, if (ret == 0) goto disabled; } - else - if (ys->ys_keyword == Y_FEATURE){ + else if (ys->ys_keyword == Y_FEATURE){ if (ys_populate_feature(h, ys) < 0) goto done; - } else switch (yang_features(h, ys)){ - case -1: /* error */ - goto done; - break; - case 0: /* disabled: remove ys */ - for (j=i+1; jys_len; j++) - yt->ys_stmt[j-1] = yt->ys_stmt[j]; - yt->ys_len--; - yt->ys_stmt[yt->ys_len] = NULL; - ys_free(ys); - continue; /* Don't increment i */ - break; - default: /* ok */ - break; - } + } + else + switch (yang_features(h, ys)){ + case -1: /* error */ + goto done; + break; + case 0: /* disabled: remove ys */ + /* Change datanodes YANG to ANYDATA, other nodes are removed + */ + if (yang_datanode(ys) && yang_config_ancestor(ys)){ + ys->ys_keyword = Y_ANYDATA; + ys_freechildren(ys); + ys->ys_len = 0; + break; + } + for (j=i+1; jys_len; j++) + yt->ys_stmt[j-1] = yt->ys_stmt[j]; + yt->ys_len--; + yt->ys_stmt[yt->ys_len] = NULL; + ys_free(ys); + continue; /* Don't increment i */ + break; + default: /* ok */ + break; + } i++; } retval = 1; @@ -3723,7 +3747,7 @@ yang_anydata_add(yang_stmt *yp, goto done; } yang_argument_set(ys, name); - if (yn_insert(yp, ys) < 0){ /* Insert into hierarchy */ + if (yp && yn_insert(yp, ys) < 0){ /* Insert into hierarchy */ ys = NULL; goto done; } diff --git a/lib/src/clixon_yang_parse_lib.c b/lib/src/clixon_yang_parse_lib.c index 6b48bc7d..432ff96f 100644 --- a/lib/src/clixon_yang_parse_lib.c +++ b/lib/src/clixon_yang_parse_lib.c @@ -264,6 +264,8 @@ yang_augment_node(clicon_handle h, /* The target node MUST be either a container, list, choice, case, input, output, or notification node. * which means it is slightly different than a schema-nodeid ? */ targetkey = yang_keyword_get(ytarget); + if (targetkey == Y_ANYDATA) + goto ok; /* Find when statement, if present */ if ((ywhen = yang_find(ys, Y_WHEN, NULL)) != NULL){ @@ -615,6 +617,9 @@ yang_expand_uses_node(yang_stmt *yn, /* Not found, try next */ if (yrt == NULL) continue; + /* Refine ANYDATA does not make sense */ + if (yang_keyword_get(yrt) == Y_ANYDATA || yang_keyword_get(yrt) == Y_ANYXML) + continue; /* Do the actual refinement */ if (ys_do_refine(yr, yrt) < 0) goto done; diff --git a/test/test_feature.sh b/test/test_feature.sh index 3d6869e1..dcd8e0d4 100755 --- a/test/test_feature.sh +++ b/test/test_feature.sh @@ -123,21 +123,31 @@ EOF # Run netconf feature test # 1: syntax node # 2: disabled or enabled +# NOTE, this was before failures when disabled, but after https://github.com/clicon/clixon/issues/322 that +# disabled nodes should be "ignored". Instead now if disabled a random node is inserted under the disabled node +# which should not work function testrun() { node=$1 enabled=$2 + new "netconf set $node" + expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<$node xmlns=\"urn:example:clixon\">foo" "" "" + + new "netconf validate $node" + expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "" + if $enabled; then - new "netconf set $node" - expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<$node xmlns=\"urn:example:clixon\">foo" "" "" + new "netconf set extra element under $node (expect fail)" + expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<$node xmlns=\"urn:example:clixon\">" "" "applicationunknown-elementkallekakaerrorFailed to find YANG spec of XML node: kallekaka with parent: $node in namespace: urn:example:clixon" + else + new "netconf set extra element under $node" + expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<$node xmlns=\"urn:example:clixon\">" "" "" new "netconf validate $node" expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "" - else - new "netconf set $node, expect fail" - expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<$node xmlns=\"urn:example:clixon\">foo" "" "applicationunknown-element$nodeerrorFailed to find YANG spec of XML node: $node with parent: config in namespace: urn:example:clixon" fi + new "netconf discard-changes" expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "" } diff --git a/test/test_feature_startup.sh b/test/test_feature_startup.sh new file mode 100755 index 00000000..5956c63d --- /dev/null +++ b/test/test_feature_startup.sh @@ -0,0 +1,101 @@ +#!/usr/bin/env bash +# Yang features. if-feature. +# The test has a example module with FEATURES A and B, where A is enabled and +# Ignore dont throw error, see https://github.com/clicon/clixon/issues/322 + +# Magic line must be first in script (see README.md) +s="$_" ; . ./lib.sh || if [ "$s" = $0 ]; then exit 0; else return 0; fi + +APPNAME=example + +cfg=$dir/conf_yang.xml +fyang=$dir/test.yang + +# Note ietf-routing@2018-03-13 assumed +cat < $cfg + + ietf-netconf:startup + $cfg + ${YANG_INSTALLDIR} + $fyang + /usr/local/lib/$APPNAME/clispec + /usr/local/lib/$APPNAME/cli + $APPNAME + /usr/local/var/$APPNAME/$APPNAME.sock + /usr/local/var/$APPNAME/$APPNAME.pidfile + $dir + +EOF + +cat < $fyang +module example{ + yang-version 1.1; + namespace "urn:example:clixon"; + prefix ex; + + feature A{ + description "This test feature is enabled"; + } + + grouping mytop { + container mycontainer { + if-feature A; + leaf myleaf { + type string; + } + } + } + uses mytop; +} +EOF + +cat< $dir/startup_db +<${DATASTORE_TOP}> + + boo + + +EOF + +# +testrun() +{ + opt=$1 + + new "kill old backend" + sudo clixon_backend -zf $cfg + if [ $? -ne 0 ]; then + err + fi + + new "start backend -s startup -f $cfg" + start_backend -s startup ${opt} -f $cfg + + new "wait backend" + wait_backend +} + + +new "test params: -f $cfg" + +new "enable feature A" +testrun "-o CLICON_FEATURE=example:A" + +new "enable feature B, expect fail" +testrun "-o CLICON_FEATURE=example:B" + +if [ $BE -ne 0 ]; then + new "kill old backend" + sudo clixon_backend -zf $cfg + if [ $? -ne 0 ]; then + err + fi + # kill backend + stop_backend -f $cfg +fi + +unset ret + +endtest + +rm -rf $dir diff --git a/yang/clixon/clixon-config@2022-03-21.yang b/yang/clixon/clixon-config@2022-03-21.yang index 09e12abd..d518b8c9 100644 --- a/yang/clixon/clixon-config@2022-03-21.yang +++ b/yang/clixon/clixon-config@2022-03-21.yang @@ -475,7 +475,9 @@ module clixon-config { use with care. The primary issue is that the unknown->anydata handling is not restricted to only loading from startup but may occur in other circumstances as well. This - means that sanity checks of erroneous XML/JSON may not be properly signalled."; + means that sanity checks of erroneous XML/JSON may not be properly signalled. + Note this is similar to what happens to YANG nodes that are disabled by a false + if-feature statement."; } leaf CLICON_BACKEND_DIR { type string;