From 4dd3f9fd346b3001e2aeb16cc442526323a3f65a Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Sat, 19 Oct 2024 17:05:19 +0200 Subject: [PATCH] Fixed: [Error when changing choice/case with different structure](https://github.com/clicon/clixon/issues/568) --- CHANGELOG.md | 1 + lib/src/clixon_datastore_write.c | 68 ++++++++-- test/test_choice.sh | 2 +- test/test_choice_implicit_delete.sh | 200 ++++++++++++++++++++++++++++ 4 files changed, 260 insertions(+), 11 deletions(-) create mode 100755 test/test_choice_implicit_delete.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b268598..5e73e8e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,6 +67,7 @@ Developers may need to change their code ### Corrected Busg +* Fixed: [Error when changing choice/case with different structure](https://github.com/clicon/clixon/issues/568) * Fixed: [Clixon handle if-feature incorrectly](https://github.com/clicon/clixon/issues/555) * Fixed: [Clixon fails to load yang with extension](https://github.com/clicon/clixon/issues/554) * Fixed: Double top-levels in xmldb_get that could occur with xpath containing choice. diff --git a/lib/src/clixon_datastore_write.c b/lib/src/clixon_datastore_write.c index 9040725d..0e19994e 100644 --- a/lib/src/clixon_datastore_write.c +++ b/lib/src/clixon_datastore_write.c @@ -109,13 +109,12 @@ struct xmldb_multi_write_arg { * @param[in] x XML node (where to look for attribute) * @param[in] name Attribute name * @param[in] ns (Expected) Namespace of attribute - * @param[in] peek If 0 dont remove attribute + * @param[in] peek If 1 dont remove attribute * @param[out] cbret Error message (if retval=0) * @param[out] valp Malloced value (if retval=1) * @retval 1 OK * @retval 0 Failed (cbret set) * @retval -1 Error - * @note as a side.effect the attribute is removed */ static int attr_ns_value(cxobj *x, @@ -384,7 +383,7 @@ check_when_condition(cxobj *x0p, goto done; } -/*! Check if x0/y0 is part of other choice/case than y1 recursively , if so purge +/*! Check if y0 is part of other choice/case than y1 recursively * * @retval 1 yes, y0 is in other case than y1 * @retval 0 No, y0 it is not in other case than y1 @@ -423,6 +422,56 @@ choice_is_other(yang_stmt *y0c, return 0; } +/*! Find/peek operation recursively other than NONE + * + * @param[in] x Base tree node + * @param[in] op NETCONF operation + * @param[out] cbret Initialized cligen buffer. Contains return XML if retval is 0. + * @retval 1 OK + * @retval 0 Fail with cbret set + * @retval -1 Error + */ +static int +find_first_op_recurse(cxobj *x, + enum operation_type *op, + cbuf *cbret) +{ + int retval = -1; + char *opstr = NULL; + cxobj *xc; + int ret; + + if (*op != OP_NONE) + goto ok; + if ((ret = attr_ns_value(x, "operation", NETCONF_BASE_NAMESPACE, 1, cbret, &opstr)) < 0) + goto done; + if (ret == 0) + goto fail; + if (opstr != NULL) + if (xml_operation(opstr, op) < 0) + goto done; + if (*op == OP_NONE){ + xc = NULL; + while ((xc = xml_child_each(x, xc, CX_ELMNT)) != NULL) { + if ((ret = find_first_op_recurse(xc, op, cbret)) < 0) + goto done; + if (ret == 0) + goto fail; + if (*op != OP_NONE) + break; + } + } + ok: + retval = 1; + done: + if (opstr) + free(opstr); + return retval; + fail: + retval = 0; + goto done; +} + /*! Check if choice nodes and if add implicitly remove all other cases, or fail if delete * * Special case is if yc parent (yp) is choice/case @@ -431,6 +480,8 @@ choice_is_other(yang_stmt *y0c, * @param[in] x0 Base tree node * @param[in] x1c Tree child * @param[in] y1c Yang spec of x1c + * @param[in] op NETCONF operation + * @param[out] cbret Initialized cligen buffer. Contains return XML if retval is 0. * @retval 1 OK * @retval 0 Fail with cbret set * @retval -1 Error @@ -441,6 +492,7 @@ choice_is_other(yang_stmt *y0c, * all nodes from all other cases. If a request creates a node from a * case, the server will delete any existing nodes that are defined in * other cases inside the choice. + * XXX just looks for first op, handling could be improved if several */ static int choice_other_match(cxobj *x0, @@ -457,16 +509,14 @@ choice_other_match(cxobj *x0, yang_stmt *y0choice; yang_stmt *y1case = NULL; yang_stmt *y1choice = NULL; - char *opstr = NULL; int ret; - if ((ret = attr_ns_value(x1c, "operation", NETCONF_BASE_NAMESPACE, 1, cbret, &opstr)) < 0) + if ((ret = find_first_op_recurse(x1c, &op, cbret)) < 0) goto done; if (ret == 0) goto fail; - if (opstr != NULL) - if (xml_operation(opstr, &op) < 0) - goto done; + if (op == OP_REMOVE || op == OP_NONE) + goto ok; if (yang_choice_case_get(y1c, &y1case, &y1choice) == 0) goto ok; x0prev = NULL; @@ -507,8 +557,6 @@ choice_other_match(cxobj *x0, ok: retval = 1; done: - if (opstr) - free(opstr); return retval; fail: retval = 0; diff --git a/test/test_choice.sh b/test/test_choice.sh index 80ca3e1e..9d9e228e 100755 --- a/test/test_choice.sh +++ b/test/test_choice.sh @@ -37,7 +37,7 @@ cat < $cfg $clidir /usr/local/var/run/$APPNAME.sock /usr/local/var/run/$APPNAME.pidfile - /usr/local/var/$APPNAME + $dir $RESTCONFIG EOF diff --git a/test/test_choice_implicit_delete.sh b/test/test_choice_implicit_delete.sh new file mode 100755 index 00000000..6338d21d --- /dev/null +++ b/test/test_choice_implicit_delete.sh @@ -0,0 +1,200 @@ +#!/usr/bin/env bash +# test of choice implicit delete, see RFC7950 Sec 7.9 3rd paragraph: +# Since only one of the choice's cases can be valid at any time in the +# data tree, the creation of a node from one case implicitly deletes +# all nodes from all other cases. If a request creates a node from a +# case, the server will delete any existing nodes that are defined in +# other cases inside the choice. + +# 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/choice.xml +clidir=$dir/cli +fyang=$dir/type.yang + +test -d ${clidir} || rm -rf ${clidir} +mkdir $clidir + +cat < $cfg + + $cfg + clixon-restconf:allow-auth-none + $dir + ${YANG_INSTALLDIR} + $fyang + /usr/local/lib/$APPNAME/clispec + /usr/local/lib/$APPNAME/cli + $APPNAME + $clidir + /usr/local/var/run/$APPNAME.sock + /usr/local/var/run/$APPNAME.pidfile + $dir + +EOF + +cat < $fyang +module system{ + yang-version 1.1; + namespace "urn:example:config"; + prefix ex; + /* Case with container vs leaf */ + container cont { + description + "One case is leaf, the other container+leaf, issue when replacing one with the other"; + choice d { + case d1 { + container d1c { + choice d11 { + leaf d11x{ + type string; + } + leaf d11y{ + type string; + } + } + } + } + case d2 { + container d2c { + leaf d2x{ + type string; + } + } + } + } + } +} +EOF + +cat < $clidir/ex.cli +# Clixon example specification +CLICON_MODE="example"; +CLICON_PROMPT="%U@%H %W> "; +CLICON_PLUGIN="example_cli"; + +# Autocli syntax tree operations +set @datamodel, cli_auto_set(); +delete("Delete a configuration item") { + @datamodel, cli_auto_del(); + all("Delete whole candidate configuration"), delete_all("candidate"); +} +validate("Validate changes"), cli_validate(); +commit("Commit the changes"), cli_commit(); +quit("Quit"), cli_quit(); +discard("Discard edits (rollback 0)"), discard_changes(); + +show("Show a particular state of the system"){ + configuration("Show configuration"), cli_show_auto_mode("candidate", "text", true, false);{ + cli("Show configuration as CLI commands"), cli_show_auto_mode("candidate", "cli", true, false, "report-all", "set "); + xml("Show configuration as XML"), cli_show_auto_mode("candidate", "xml", true, false, NULL); + } +} +EOF + +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 + sudo pkill -f clixon_backend # to be sure + + new "start backend -s init -f $cfg" + start_backend -s init -f $cfg +fi + +new "wait backend" +wait_backend + +# replace + +new "set d1" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" " + + +aaa + + +" "" "" + +new "replace d2 top" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "none + +bbb + +" "" "" + +new "check d2" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "bbb" "" + +new "replace d1 part" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "none + +ccc + +" "" "" + +new "check d1" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "ccc" "" + +# merge +new "merge d2 part" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "none + +ddd + +" "" "" + +new "check d2" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "ddd" "" + +# create +new "create d1 part" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "none + +eee + +" "" "" + +new "check d1" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "eee" "" + +# remove +new "remove d2 part" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "none + +ddd + +" "" "" + +new "check d1" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "eee" "" + +# delete +new "delete d2 part" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "none + +ddd + +" "" "applicationdata-missingerrorData does not exist; cannot delete resource" + +if [ $BE -ne 0 ]; then + 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 +fi + +rm -rf $dir + +new "endtest" +endtest