* 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

@@ -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
+  * `clicon_rpc_commit()` and `cli_commit`
-    `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)

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:

apps/snmp/snmp_handler.c

@@ -44,7 +44,6 @@
 #include <errno.h>
 #include <sys/types.h>
 #include <signal.h>
-#include <assert.h>
 
 /* net-snmp */
 #include <net-snmp/net-snmp-config.h>
@@ -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)
+	if ((ret = clicon_rpc_commit(sh->sh_h, 0, 0, 0, NULL, NULL)) < 0)
-	    goto done;	
+	    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)
+	if ((ret = clicon_rpc_commit(sh->sh_h, 0, 0, 0, NULL, NULL)) < 0)
-	    goto done;	
+	    goto done;
+	if (ret == 0){
+	    clicon_rpc_discard_changes(sh->sh_h);
+	    goto done;
+	}
 	break;
     case MODE_SET_FREE:     // 4
 	break;

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          OK
- * @retval   -1          Error and logged to syslog
+ * @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)

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 <<EOF > $cfg
   <CLICON_YANG_MAIN_FILE>$fyang</CLICON_YANG_MAIN_FILE>
   <CLICON_SOCK>$dir/$APPNAME.sock</CLICON_SOCK>
   <CLICON_BACKEND_PIDFILE>/var/tmp/$APPNAME.pidfile</CLICON_BACKEND_PIDFILE>
+  <CLICON_BACKEND_DIR>/usr/local/lib/$APPNAME/backend</CLICON_BACKEND_DIR>
   <CLICON_XMLDB_DIR>$dir</CLICON_XMLDB_DIR>
   <CLICON_SNMP_AGENT_SOCK>unix:$SOCK</CLICON_SNMP_AGENT_SOCK>
   <CLICON_SNMP_MIB>CLIXON-TYPES-MIB</CLICON_SNMP_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</$name>"    
 
+# 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