diff --git a/CHANGELOG.md b/CHANGELOG.md index 9dfc3a95..e9c69b0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,8 +46,14 @@ Users may have to change how they access the system ### Minor changes +* * Added sanity check that a yang module name matches the filename +### Corrected Bugs + +* Fixed: [default state data returned with get-config](https://github.com/clicon/clixon/issues/140) + * Generalized default code for both config and state + ## 4.7.0 14 September 2020 diff --git a/apps/backend/backend_client.c b/apps/backend/backend_client.c index f9ea4692..ccdb7f70 100644 --- a/apps/backend/backend_client.c +++ b/apps/backend/backend_client.c @@ -324,6 +324,13 @@ client_statedata(clicon_handle h, clicon_err(OE_UNIX, errno, "cbuf_new"); goto done; } + /* Add default state to config if present */ + if (xml_default_recurse(*xret, 1) < 0) + goto done; + /* Add default global state */ + if (xml_global_defaults(h, *xret, nsc, xpath, yspec, 1) < 0) + goto done; + if (clicon_option_bool(h, "CLICON_STREAM_DISCOVERY_RFC5277")){ if ((ymod = yang_find_module_by_name(yspec, "clixon-rfc5277")) == NULL){ clicon_err(OE_YANG, ENOENT, "yang module clixon-rfc5277 not found"); @@ -367,6 +374,7 @@ client_statedata(clicon_handle h, if (ret == 0) goto fail; } + /* Use plugin state callbacks */ if ((ret = clixon_plugin_statedata_all(h, yspec, nsc, xpath, xret)) < 0) goto done; if (ret == 0) diff --git a/apps/backend/backend_commit.c b/apps/backend/backend_commit.c index e65e7399..cc1f625e 100644 --- a/apps/backend/backend_commit.c +++ b/apps/backend/backend_commit.c @@ -254,10 +254,10 @@ startup_common(clicon_handle h, if (xml_sort_recurse(xt) < 0) goto done; /* Add global defaults. */ - if (xml_global_defaults(h, xt, NULL, NULL, yspec) < 0) + if (xml_global_defaults(h, xt, NULL, NULL, yspec, 0) < 0) goto done; /* Apply default values (removed in clear function) */ - if (xml_default_recurse(xt) < 0) + if (xml_default_recurse(xt, 0) < 0) goto done; /* Handcraft transition with with only add tree */ diff --git a/apps/backend/backend_plugin.c b/apps/backend/backend_plugin.c index e02270c7..68433597 100644 --- a/apps/backend/backend_plugin.c +++ b/apps/backend/backend_plugin.c @@ -279,7 +279,7 @@ clixon_plugin_statedata_all(clicon_handle h, goto done; if (ret == 0){ if (clixon_netconf_internal_error(xerr, - ". Internal error, state callback returned invalid XML: ", + ". Internal error, state callback returned invalid XML from plugin: ", cp->cp_name) < 0) goto done; xml_free(*xret); @@ -289,7 +289,16 @@ clixon_plugin_statedata_all(clicon_handle h, } if (xml_sort_recurse(x) < 0) goto done; - if (xml_default_recurse(x) < 0) + /* Mark non-presence containers as XML_FLAG_DEFAULT */ + if (xml_apply(x, CX_ELMNT, xml_nopresence_default_mark, (void*)XML_FLAG_DEFAULT) < 0) + goto done; + /* Clear XML tree of defaults */ + if (xml_tree_prune_flagged(x, XML_FLAG_DEFAULT, 1) < 0) + goto done; + /* clear mark and change */ + xml_apply0(x, CX_ELMNT, (xml_applyfn_t*)xml_flag_reset, + (void*)(0xffff)); + if (xml_default_recurse(x, 1) < 0) goto done; if ((ret = netconf_trymerge(x, yspec, xret)) < 0) goto done; diff --git a/lib/clixon/clixon_xml_map.h b/lib/clixon/clixon_xml_map.h index 55e26faa..75292014 100644 --- a/lib/clixon/clixon_xml_map.h +++ b/lib/clixon/clixon_xml_map.h @@ -64,9 +64,8 @@ int xml_diff(yang_stmt *yspec, cxobj *x0, cxobj *x1, int xml_tree_prune_flagged_sub(cxobj *xt, int flag, int test, int *upmark); int xml_tree_prune_flagged(cxobj *xt, int flag, int test); int xml_namespace_change(cxobj *x, char *ns, char *prefix); -int xml_default(cxobj *x); -int xml_default_recurse(cxobj *xn); -int xml_global_defaults(clicon_handle h, cxobj *xn, cvec *nsc, const char *xpath, yang_stmt *yspec); +int xml_default_recurse(cxobj *xn, int state); +int xml_global_defaults(clicon_handle h, cxobj *xn, cvec *nsc, const char *xpath, yang_stmt *yspec, int state); int xml_nopresence_default(cxobj *xt); int xml_nopresence_default_mark(cxobj *x, void *arg); int xml_sanity(cxobj *x, void *arg); diff --git a/lib/src/clixon_datastore_read.c b/lib/src/clixon_datastore_read.c index 9f18e5c0..7fdc8e8e 100644 --- a/lib/src/clixon_datastore_read.c +++ b/lib/src/clixon_datastore_read.c @@ -568,10 +568,10 @@ xmldb_get_nocache(clicon_handle h, if (yb != YB_NONE){ /* Add global defaults. */ - if (xml_global_defaults(h, xt, nsc, xpath, yspec) < 0) + if (xml_global_defaults(h, xt, nsc, xpath, yspec, 0) < 0) goto done; /* Add default values (if not set) */ - if (xml_default_recurse(xt) < 0) + if (xml_default_recurse(xt, 0) < 0) goto done; } /* If empty NACM config, then disable NACM if loaded @@ -715,10 +715,10 @@ xmldb_get_cache(clicon_handle h, goto done; if (yb != YB_NONE){ /* Add default global values */ - if (xml_global_defaults(h, x1t, nsc, xpath, yspec) < 0) + if (xml_global_defaults(h, x1t, nsc, xpath, yspec, 0) < 0) goto done; /* Add default recursive values */ - if (xml_default_recurse(x1t) < 0) + if (xml_default_recurse(x1t, 0) < 0) goto done; } /* If empty NACM config, then disable NACM if loaded @@ -812,10 +812,10 @@ xmldb_get_zerocopy(clicon_handle h, } if (yb != YB_NONE){ /* Add global defaults. */ - if (xml_global_defaults(h, x0t, nsc, xpath, yspec) < 0) + if (xml_global_defaults(h, x0t, nsc, xpath, yspec, 0) < 0) goto done; /* Apply default values (removed in clear function) */ - if (xml_default_recurse(x0t) < 0) + if (xml_default_recurse(x0t, 0) < 0) goto done; } /* If empty NACM config, then disable NACM if loaded diff --git a/lib/src/clixon_options.c b/lib/src/clixon_options.c index 0380f26b..5d30288d 100644 --- a/lib/src/clixon_options.c +++ b/lib/src/clixon_options.c @@ -260,7 +260,7 @@ parse_configfile(clicon_handle h, goto done; } - if (xml_default_recurse(xc) < 0) + if (xml_default_recurse(xc, 0) < 0) goto done; if ((ret = xml_yang_validate_add(h, xc, &xerr)) < 0) goto done; diff --git a/lib/src/clixon_validate.c b/lib/src/clixon_validate.c index 0eb67e8c..8fda734d 100644 --- a/lib/src/clixon_validate.c +++ b/lib/src/clixon_validate.c @@ -347,7 +347,7 @@ xml_yang_validate_rpc(clicon_handle h, goto done; /* error or validation fail */ if ((retval = xml_yang_validate_add(h, xn, xret)) < 1) goto done; /* error or validation fail */ - if (xml_default_recurse(xn) < 0) + if (xml_default_recurse(xn, 0) < 0) goto done; } // ok: /* pass validation */ diff --git a/lib/src/clixon_xml_map.c b/lib/src/clixon_xml_map.c index 9af1bb8b..a599b1db 100644 --- a/lib/src/clixon_xml_map.c +++ b/lib/src/clixon_xml_map.c @@ -1118,12 +1118,14 @@ xml_nopresence_try(yang_stmt *yt, * XML containers may be created to host default values. That code may be a little too recursive. * @param[in] yt Yang spec * @param[in] xt XML tree (with yt as spec of xt, informally) + * @param[in] state Set if global state, otherwise config * @retval 0 OK * @retval -1 Error */ static int xml_default1(yang_stmt *yt, - cxobj *xt) + cxobj *xt, + int state) { int retval = -1; yang_stmt *yc; @@ -1145,6 +1147,8 @@ xml_default1(yang_stmt *yt, case Y_OUTPUT: yc = NULL; while ((yc = yn_each(yt, yc)) != NULL) { + if (!state && !yang_config(yc)) + continue; switch (yang_keyword_get(yc)){ case Y_LEAF: if (!cv_flag(yang_cv_get(yc), V_UNSET)){ /* Default value exists */ @@ -1173,7 +1177,7 @@ xml_default1(yang_stmt *yt, goto done; xml_sort(xt); /* Then call it recursively */ - if (xml_default1(yc, xc) < 0) + if (xml_default1(yc, xc, state) < 0) goto done; } } @@ -1196,11 +1200,13 @@ xml_default1(yang_stmt *yt, * * Assume yang is bound to the tree * @param[in] xt XML tree + * @param[in] state If set expand defaults also for state data, otherwise only config * @retval 0 OK * @retval -1 Error */ -int -xml_default(cxobj *xt) +static int +xml_default(cxobj *xt, + int state) { int retval = -1; yang_stmt *ys; @@ -1209,7 +1215,7 @@ xml_default(cxobj *xt) retval = 0; goto done; } - if (xml_default1(ys, xt) < 0) + if (xml_default1(ys, xt, state) < 0) goto done; retval = 0; done: @@ -1218,20 +1224,27 @@ xml_default(cxobj *xt) /*! Recursively fill in default values in an XML tree * @param[in] xt XML tree + * @param[in] state If set expand defaults also for state data, otherwise only config * @retval 0 OK * @retval -1 Error */ int -xml_default_recurse(cxobj *xn) +xml_default_recurse(cxobj *xn, + int state) { - int retval = -1; - cxobj *x; + int retval = -1; + cxobj *x; + yang_stmt *y; - if (xml_default(xn) < 0) + if (xml_default(xn, state) < 0) goto done; x = NULL; while ((x = xml_child_each(xn, x, CX_ELMNT)) != NULL) { - if (xml_default_recurse(x) < 0) + if ((y = (yang_stmt*)xml_spec(x)) != NULL){ + if (!state && !yang_config(y)) + continue; + } + if (xml_default_recurse(x, state) < 0) goto done; } retval = 0; @@ -1242,15 +1255,16 @@ xml_default_recurse(cxobj *xn) /*! Expand and set default values of global top-level on XML tree * * Not recursive, except in one case with one or several non-presence containers - * @param[in] h Clixon handle - * @param[in] yspec Top-level YANG specification tree, all modules * @param[in] xt XML tree + * @param[in] yspec Top-level YANG specification tree, all modules + * @param[in] state Set if global state, otherwise config * @retval 0 OK * @retval -1 Error */ static int xml_global_defaults_create(cxobj *xt, - yang_stmt *yspec) + yang_stmt *yspec, + int state) { int retval = -1; @@ -1261,7 +1275,7 @@ xml_global_defaults_create(cxobj *xt, goto done; } while ((ymod = yn_each(yspec, ymod)) != NULL) - if (xml_default1(ymod, xt) < 0) + if (xml_default1(ymod, xt, state) < 0) goto done; retval = 0; done: @@ -1275,6 +1289,7 @@ xml_global_defaults_create(cxobj *xt, * @param[in] xt XML tree, assume already filtered with xpath * @param[in] xpath Filter global defaults with this and merge with xt * @param[in] yspec Top-level YANG specification tree, all modules + * @param[in] state Set if global state, otherwise config * @retval 0 OK * @retval -1 Error * Uses cache? @@ -1284,7 +1299,8 @@ xml_global_defaults(clicon_handle h, cxobj *xt, cvec *nsc, const char *xpath, - yang_stmt *yspec) + yang_stmt *yspec, + int state) { int retval = -1; db_elmnt de0 = {0,}; @@ -1296,16 +1312,19 @@ xml_global_defaults(clicon_handle h, int i; cxobj *x0; int ret; + char *key; + /* Use different keys for config and state */ + key = state ? "global-defaults-state" : "global-defaults-config"; /* First get or compute global xml tree cache */ - if ((de = clicon_db_elmnt_get(h, "global-defaults")) == NULL){ - /* Create it it */ + if ((de = clicon_db_elmnt_get(h, key)) == NULL){ + /* Create it */ if ((xcache = xml_new("config", NULL, CX_ELMNT)) == NULL) goto done; - if (xml_global_defaults_create(xcache, yspec) < 0) + if (xml_global_defaults_create(xcache, yspec, state) < 0) goto done; de0.de_xml = xcache; - clicon_db_elmnt_set(h, "global-defaults", &de0); + clicon_db_elmnt_set(h, key, &de0); } else xcache = de->de_xml; @@ -1323,6 +1342,7 @@ xml_global_defaults(clicon_handle h, xml_flag_set(x0, XML_FLAG_MARK); xml_apply_ancestor(x0, (xml_applyfn_t*)xml_flag_set, (void*)XML_FLAG_CHANGE); } + /* Create a new tree and copy over the parts from the cache that matches xpath */ if ((xpart = xml_new("config", NULL, CX_ELMNT)) == NULL) goto done; if (xml_copy_marked(xcache, xpart) < 0) /* config */ diff --git a/test/test_augment_state.sh b/test/test_augment_state.sh new file mode 100755 index 00000000..975d1ec2 --- /dev/null +++ b/test/test_augment_state.sh @@ -0,0 +1,263 @@ +#!/usr/bin/env bash +# Test augmented state +# Use main example -- -sS option to add state via a file +# +# 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/example-augment.yang +fyang2=$dir/example-lib.yang +fstate=$dir/state.xml + +cat < $cfg + + $cfg + a:test + $dir + /usr/local/share/clixon + $fyang + /usr/local/lib/$APPNAME/clispec + /usr/local/lib/$APPNAME/cli + $APPNAME + /usr/local/var/$APPNAME/$APPNAME.sock + /usr/local/var/$APPNAME/$APPNAME.pidfile + /usr/local/lib/$APPNAME/backend + false + /usr/local/var/$APPNAME + false + +EOF + +# This is the lib function with the base container +cat < $fyang2 +module example-lib { + yang-version 1.1; + namespace "urn:example:lib"; + revision "2019-03-04"; + prefix lib; + container global-state { + config false; + leaf gbds{ + description "Global base default state"; + type string; + default "gbds"; + } + leaf gbos{ + description "Global base optional state"; + type string; + } + container nopres{ + description "This should be removed"; + } + } + container base-config { + list parameter{ + key name; + leaf name{ + type string; + } + container param-state { + config false; + leaf lbds{ + description "Local base default state"; + type string; + default "lbds"; + } + leaf lbos{ + description "Local base optional state"; + type string; + } + } + } + } +} +EOF + +# This is the main module where the augment exists +cat < $fyang +module example-augment { + yang-version 1.1; + namespace "urn:example:augment"; + prefix aug; + revision "2020-09-25"; + import example-lib { + prefix lib; + } + /* Augments global state */ + augment "/lib:global-state" { + leaf gads{ + description "Global augmented default state"; + type string; + default "gads"; + } + leaf gaos{ + description "Global augmented optional state"; + type string; + } + } + /* Augments state in config in-line */ + augment "/lib:base-config/lib:parameter/lib:param-state" { + leaf lads{ + description "Local augmented default state"; + type string; + default "lads"; + } + leaf laos{ + description "Local augmented optional state"; + type string; + } + } +} +EOF + +# Get config and state +# Arguments +# - expected config +# - expected state +testrun() +{ + config=$1 + state=$2 + + new "get config" + if [ -z "$config" ]; then + expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "^]]>]]>$" + else + expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "^$config]]>]]>$" + fi + + new "get state" + expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "^$state]]>]]>$" +} + +new "test params: -f $cfg" + +if [ $BE -ne 0 ]; then + new "kill old backend" + sudo clixon_backend -zf $cfg + if [ $? -ne 0 ]; then + err + fi + new "start backend -s init -f $cfg -- -sS $fstate" + start_backend -s init -f $cfg -- -sS $fstate +fi +new "waiting" +wait_backend + + +#----------------------------- +new "1. Empty config/state, expect global default state" + +CONFIG="" + +cat < $fstate +EOF + +EXPSTATE=$(cat <gbdsgads +EOF +) + +testrun "$CONFIG" "$EXPSTATE" + +#----------------------------- +new "2. Empty config/top-level state, expect global default state" +cat < $fstate + +EOF + +testrun "$CONFIG" "$EXPSTATE" + +#----------------------------- +new "3. Empty config/top-level w non-presence state, expect global default state" +cat < $fstate + + + +EOF + +testrun "$CONFIG" "$EXPSTATE" + +#----------------------------- +new "4. Empty config + optional state, expect global default + optional state" +cat < $fstate + + gbos + gaos + +EOF + +EXPSTATE=$(cat <gbdsgbosgadsgaos +EOF +) + +testrun "$CONFIG" "$EXPSTATE" + +#----------------------------- +# From here, add a config tree +new "5. Config tree, empty top state, expect default top state and default local state" + +CONFIG=$(cat <a +EOF +) + +cat < $fstate +EOF + +EXPSTATE=$(cat <gbdsgadsalbdslads +EOF +) + +new "Add config" +expecteof "$clixon_netconf -qf $cfg" 0 "$CONFIG]]>]]>" "^]]>]]>" + +new "netconf commit" +expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "^]]>]]>$" + +testrun "$CONFIG" "$EXPSTATE" + +#----------------------------- + +new "6. Config tree and optional tree state, empty top state, expect default top state and default local state" + +cat < $fstate + + + a + + lbos + laos + + + +EOF + +EXPSTATE=$(cat <gbdsgadsalbdslbosladslaos +EOF +) + +testrun "$CONFIG" "$EXPSTATE" + +#----------------------------- + +if [ $BE -eq 0 ]; then + exit # BE +fi + +new "Kill backend" +# Check if premature kill +pid=$(pgrep -u root -f clixon_backend) +if [ -z "$pid" ]; then + err "backend already dead" +fi +# kill backend +stop_backend -f $cfg + +rm -rf $dir diff --git a/test/test_restconf_err.sh b/test/test_restconf_err.sh index 6e64d319..4264a0d4 100755 --- a/test/test_restconf_err.sh +++ b/test/test_restconf_err.sh @@ -227,7 +227,7 @@ expectpart "$(curl $CURLOPTS -X GET $RCPROTO://localhost/restconf/data/augment:r #---------------------------------------------- # Also generate an invalid state XML. This should generate an "Internal" error and the name of the new "restconf GET failed state" -expectpart "$(curl $CURLOPTS -X GET -H 'Accept: application/yang-data+xml' $RCPROTO://localhost/restconf/data?content=nonconfig)" 0 '412 Precondition Failed' 'applicationoperation-failedmystateerrorFailed to find YANG spec of XML node: mystate with parent: top in namespace: urn:example:foobar. Internal error, state callback returned invalid XML: example_backend' +expectpart "$(curl $CURLOPTS -X GET -H 'Accept: application/yang-data+xml' $RCPROTO://localhost/restconf/data?content=nonconfig)" 0 '412 Precondition Failed' 'applicationoperation-failedmystateerrorFailed to find YANG spec of XML node: mystate with parent: config in namespace: urn:example:foobar. Internal error, state callback returned invalid XML from plugin: example_backend' # Add error XML a[4242] , it should fail on autocommit but may not be discarded, therefore still # there in candidate when want to add something else diff --git a/test/test_yang_anydata.sh b/test/test_yang_anydata.sh index 89796d6f..ba18261d 100755 --- a/test/test_yang_anydata.sh +++ b/test/test_yang_anydata.sh @@ -227,7 +227,7 @@ EOF echo "$STATE0" > $fstate new "Get state (negative test)" - expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "error-message>Failed to find YANG spec of XML node: u5 with parent: sb in namespace: urn:example:unknown. Internal error, state callback returned invalid XML: example_backend" + expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "error-message>Failed to find YANG spec of XML node: u5 with parent: sb in namespace: urn:example:unknown. Internal error, state callback returned invalid XML from plugin: example_backend" new "restconf get state(negative)" expectpart "$(curl $CURLOPTS -X GET -H "Accept: application/yang-data+xml" $RCPROTO://localhost/restconf/data?content=nonconfig)" 0 "HTTP/1.1 412 Precondition Failed" "operation-failedu5" diff --git a/util/clixon_util_path.c b/util/clixon_util_path.c index 32d6b9c1..209b9acd 100644 --- a/util/clixon_util_path.c +++ b/util/clixon_util_path.c @@ -241,7 +241,7 @@ main(int argc, if (xml_apply0(x, -1, xml_sort_verify, h) < 0) clicon_log(LOG_NOTICE, "%s: sort verify failed", __FUNCTION__); /* Add default values */ - if (xml_default_recurse(x) < 0) + if (xml_default_recurse(x, 0) < 0) goto done; if ((ret = xml_yang_validate_all_top(h, x, &xerr)) < 0) goto done; diff --git a/util/clixon_util_xml.c b/util/clixon_util_xml.c index ca852297..d651db97 100644 --- a/util/clixon_util_xml.c +++ b/util/clixon_util_xml.c @@ -80,7 +80,7 @@ validate_tree(clicon_handle h, /* should already be populated */ /* Add default values */ - if (xml_default_recurse(xt) < 0) + if (xml_default_recurse(xt, 0) < 0) goto done; if (xml_apply(xt, -1, xml_sort_verify, h) < 0) clicon_log(LOG_NOTICE, "%s: sort verify failed", __FUNCTION__); diff --git a/util/clixon_util_xpath.c b/util/clixon_util_xpath.c index 14c5137c..58de8f55 100644 --- a/util/clixon_util_xpath.c +++ b/util/clixon_util_xpath.c @@ -323,7 +323,7 @@ main(int argc, goto done; /* Add default values */ - if (xml_default_recurse(x0) < 0) + if (xml_default_recurse(x0, 0) < 0) goto done; if (xml_apply0(x0, -1, xml_sort_verify, h) < 0) clicon_log(LOG_NOTICE, "%s: sort verify failed", __FUNCTION__);