From 575429caa100325d714809d90fd654298e3c7a52 Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Sat, 8 Oct 2022 17:04:02 +0200 Subject: [PATCH] * Fixed: [Non-obvious behavior of clixon_snmp after snmpset command when transaction validation returns an error](https://github.com/clicon/clixon/issues/375) * Changed `clicon_rpc_commit()` and `clicon_rpc_validate`: Added three-value return. --- CHANGELOG.md | 10 ++++++--- apps/cli/cli_common.c | 4 ++-- apps/snmp/snmp_handler.c | 18 +++++++++++------ lib/src/clixon_proto_client.c | 20 ++++++++++-------- test/test_snmp_set.sh | 38 ++++++++++++++++++++++++++++++++++- 5 files changed, 70 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c88f4001..affb827d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,15 +66,19 @@ Developers may need to change their code * C API changes * Added `defaults` parameter to `clicon_rpc_get_pageable_list()` - * Added `confirmed`, `cancel`, `timeout`, `persist-id`, and `persist-id-val` parameters to - `cli_commit(...)` - + * `clicon_rpc_commit()` and `cli_commit` + * Added `confirmed`, `cancel`, `timeout`, `persist-id`, and `persist-id-val` parameters to + * `clicon_rpc_commit()` and `clicon_rpc_validate` + * Added three-value return. + * Code need to be changed from: checking for `<0` to `<1` to keep same semantics + ### Minor features * Added warning if modstate is not present in datastore if `CLICON_XMLDB_MODSTATE` is set. ### Corrected Bugs +* Fixed: [Non-obvious behavior of clixon_snmp after snmpset command when transaction validation returns an error](https://github.com/clicon/clixon/issues/375) * Fixed: [clixon_snmp module crashes on snmpwalk command](https://github.com/clicon/clixon/issues/378) * Fixed: [unneeded trailing zero character on SNMP strings](https://github.com/clicon/clixon/issues/367) * Fixed: [message-id present on netconf app "hello"](https://github.com/clicon/clixon/issues/369) diff --git a/apps/cli/cli_common.c b/apps/cli/cli_common.c index 158921b6..574eb9b4 100644 --- a/apps/cli/cli_common.c +++ b/apps/cli/cli_common.c @@ -664,7 +664,7 @@ cli_commit(clicon_handle h, persist = cvec_find_str(vars, "persist-val"); persist_id = cvec_find_str(vars, "persist-id-val"); - if ((retval = clicon_rpc_commit(h, confirmed, cancel, timeout, persist, persist_id)) < 0) + if (clicon_rpc_commit(h, confirmed, cancel, timeout, persist, persist_id) < 1) goto done; retval = 0; done: @@ -680,7 +680,7 @@ cli_validate(clicon_handle h, { int retval = -1; - if ((retval = clicon_rpc_validate(h, "candidate")) < 0) + if (clicon_rpc_validate(h, "candidate") < 1) goto done; retval = 0; done: diff --git a/apps/snmp/snmp_handler.c b/apps/snmp/snmp_handler.c index 183e5515..1f46dd64 100644 --- a/apps/snmp/snmp_handler.c +++ b/apps/snmp/snmp_handler.c @@ -44,7 +44,6 @@ #include #include #include -#include /* net-snmp */ #include @@ -730,7 +729,6 @@ clixon_snmp_scalar_handler1(netsnmp_mib_handler *handler, goto done; break; case MODE_GETNEXT: /* 161 */ - assert(0); // Not seen? break; case MODE_SET_RESERVE1: /* 0 */ /* Translate from YANG ys leaf type to SNMP asn1.1 type ids (not value), also cvtype */ @@ -751,8 +749,12 @@ clixon_snmp_scalar_handler1(netsnmp_mib_handler *handler, goto done; break; case MODE_SET_COMMIT: /* 3 */ - if (clicon_rpc_commit(sh->sh_h, 0, 0, 0, NULL, NULL) < 0) - goto done; + if ((ret = clicon_rpc_commit(sh->sh_h, 0, 0, 0, NULL, NULL)) < 0) + goto done; + if (ret == 0){ + clicon_rpc_discard_changes(sh->sh_h); + goto done; + } break; case MODE_SET_FREE: /* 4 */ break; @@ -1304,8 +1306,12 @@ clixon_snmp_table_handler1(netsnmp_mib_handler *handler, } break; case MODE_SET_COMMIT: // 3 - if (clicon_rpc_commit(sh->sh_h, 0, 0, 0, NULL, NULL) < 0) - goto done; + if ((ret = clicon_rpc_commit(sh->sh_h, 0, 0, 0, NULL, NULL)) < 0) + goto done; + if (ret == 0){ + clicon_rpc_discard_changes(sh->sh_h); + goto done; + } break; case MODE_SET_FREE: // 4 break; diff --git a/lib/src/clixon_proto_client.c b/lib/src/clixon_proto_client.c index 4b2b1c9b..fc6a20cc 100644 --- a/lib/src/clixon_proto_client.c +++ b/lib/src/clixon_proto_client.c @@ -1225,8 +1225,10 @@ clicon_rpc_kill_session(clicon_handle h, /*! Send validate request to backend daemon * @param[in] h CLICON handle * @param[in] db Name of database - * @retval 0 OK + * @retval 1 OK + * @retval 0 Invalid, netconf error return, and logged to syslog * @retval -1 Error and logged to syslog + * @note error returns are logged but not returned */ int clicon_rpc_validate(clicon_handle h, @@ -1253,9 +1255,10 @@ clicon_rpc_validate(clicon_handle h, goto done; if ((xerr = xpath_first(xret, NULL, "//rpc-error")) != NULL){ clixon_netconf_error(xerr, CLIXON_ERRSTR_VALIDATE_FAILED, NULL); + retval = 0; goto done; } - retval = 0; + retval = 1; done: if (msg) free(msg); @@ -1271,9 +1274,11 @@ clicon_rpc_validate(clicon_handle h, * @param[in] timeout For confirmed, a timeout in seconds (default 600s) * @param[in] persist A persist identifier to use * @param[in] persist_id If cancel or confirmed, the persist id - * @retval 0 OK - * @retval -1 Error and logged to syslog + * @retval 1 OK + * @retval 0 Invalid, netconf error return, and logged to syslog + * @retval -1 Error * @see rfc6241 Sec 8.4 Confirmed Commit Capability + * @note error returns are logged but not returned */ int clicon_rpc_commit(clicon_handle h, @@ -1318,8 +1323,6 @@ clicon_rpc_commit(clicon_handle h, }; sprintf(timeout_xml, TIMEOUT_XML_FMT, timeout); } - - if (session_id_check(h, &session_id) < 0) goto done; username = clicon_username_get(h); @@ -1353,9 +1356,10 @@ clicon_rpc_commit(clicon_handle h, goto done; if ((xerr = xpath_first(xret, NULL, "//rpc-error")) != NULL){ clixon_netconf_error(xerr, CLIXON_ERRSTR_COMMIT_FAILED, NULL); + retval = 0; goto done; } - retval = 0; + retval = 1; done: if (xret) xml_free(xret); @@ -1550,7 +1554,7 @@ clicon_rpc_restconf_debug(clicon_handle h, clicon_err(OE_XML, 0, "rpc error"); /* XXX extract info from rpc-error */ goto done; } - if ((retval = clicon_rpc_commit(h, 0, 0, 0, NULL, NULL)) < 0) + if ((retval = clicon_rpc_commit(h, 0, 0, 0, NULL, NULL)) < 1) goto done; done: if (msg) diff --git a/test/test_snmp_set.sh b/test/test_snmp_set.sh index f57ff676..54a13152 100755 --- a/test/test_snmp_set.sh +++ b/test/test_snmp_set.sh @@ -2,6 +2,7 @@ # snmpset. This requires deviation of MIB-YANG to make write operations # Get default value, set new value via SNMP and check it, set new value via NETCONF and check # Selected types from CLIXON/IF-MIB/ENTITY mib +# Also an incomplete commit failed test # Magic line must be first in script (see README.md) s="$_" ; . ./lib.sh || if [ "$s" = $0 ]; then exit 0; else return 0; fi @@ -33,6 +34,7 @@ cat < $cfg $fyang $dir/$APPNAME.sock /var/tmp/$APPNAME.pidfile + /usr/local/lib/$APPNAME/backend $dir unix:$SOCK CLIXON-TYPES-MIB @@ -195,9 +197,10 @@ function testrun() echo "$snmpset $oid $set_type $value" expectpart "$($snmpset $oid $set_type $value)" 0 "$type:" "$value" else - echo "$snmpset $oid $set_type $value" + echo "$snmpset $oid $set_type $value2" expectpart "$($snmpset $oid $set_type $value)" 0 "$type: $value2" fi + new "Check $name via SNMP" if [ "$type" == "STRING" ]; then expectpart "$($snmpget $oid)" 0 "$type:" "$value" @@ -211,6 +214,17 @@ function testrun() function testexit(){ stop_snmp + + 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 } new "SNMP tests" @@ -241,6 +255,28 @@ expectpart "$($snmpget $oid)" 0 "$type: active(1)" new "Check $name via CLI" expectpart "$($clixon_cli -1 -f $cfg show config xml)" 0 "<$name>active" +# restart backend with synthetic validation failure +# Incomplete commit failed test: the commit fails by logging but this is not actually checked +if [ $BE -ne 0 ]; then + # Kill old backend and start a new one + new "kill old backend" + sudo clixon_backend -zf $cfg + if [ $? -ne 0 ]; then + err "Failed to start backend" + fi + + sudo pkill -f clixon_backend + + new "Starting backend" + start_backend -s startup -f $cfg -- -V CLIXON-TYPES-MIB/clixonExampleScalars/clixonExampleInteger +fi + +new "wait backend" +wait_backend + +new "set value with error" +expectpart "$($snmpset ${MIB}.1.1 i 4321)" 0 "INTEGER: 4321" + new "Cleaning up" testexit