* Fixed: [Non-obvious behavior of clixon_snmp after snmpset command when transaction validation returns an error](https://github.com/clicon/clixon/issues/375)

* Fixed by validating writes on ACTION instead of COMMIT since libnetsnmp seems not to accept commit errors

CHANGELOG.md

@@ -81,6 +81,8 @@ Developers may need to change their code
 
 ### 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 by validating writes on ACTION instead of COMMIT since libnetsnmp seems not to accept commit errors
 * Fixed: [YANG when condition evaluated as false combined with a mandatory leaf does not work](https://github.com/clicon/clixon/issues/380)
 * Fixed: [Trying to change the "read-only" node through snmpset](https://github.com/clicon/clixon/issues/376)
 * Fixed: [Trying to change the "config false" node through snmpset](https://github.com/clicon/clixon/issues/377)

apps/snmp/snmp_handler.c

@@ -733,8 +733,8 @@ clixon_snmp_scalar_handler1(netsnmp_mib_handler          *handler,
     case MODE_SET_RESERVE1: /* 0 */
 	if (!yang_config_ancestor(sh->sh_ys) ||
 	    nhreg->modes == HANDLER_CAN_RONLY){
-	    retval = SNMP_ERR_NOTWRITABLE;
+	    netsnmp_request_set_error(request, SNMP_ERR_NOTWRITABLE);
-	    goto done;;	    
+	    goto done;
 	}
 	/* Translate from YANG ys leaf type to SNMP asn1.1 type ids (not value), also cvtype */
 	if (type_yang2asn1(sh->sh_ys, &asn1_type, 0) < 0)
@@ -752,13 +752,28 @@ clixon_snmp_scalar_handler1(netsnmp_mib_handler          *handler,
     case MODE_SET_ACTION:   /* 2 */
 	if (snmp_scalar_set(sh->sh_h, sh->sh_ys, NULL, NULL, reqinfo, request) < 0)
 	    goto done;
+	/*
+	 * There does not seem to be a separate validation action and commit does not 
+	 * return an error.
+	 * Therefore validation is done here directly as well as discard if it fails.
+	 */
+	if ((ret = clicon_rpc_validate(sh->sh_h, "candidate")) < 0)
+	    goto done;
+	if (ret == 0){
+	    clicon_rpc_discard_changes(sh->sh_h);
+	    netsnmp_request_set_error(request, SNMP_ERR_COMMITFAILED);
+	    goto done;
+	}
         break;
     case MODE_SET_COMMIT:   /* 3 */
 	if ((ret = clicon_rpc_commit(sh->sh_h, 0, 0, 0, NULL, NULL)) < 0)
 	    goto done;
 	if (ret == 0){
+	    /* Note that error given in commit is not propagated to the snmp client,
+	     * therefore validation is in the ACTION instead
+	     */
 	    clicon_rpc_discard_changes(sh->sh_h);
-	    netsnmp_request_set_error(request, SNMP_ERR_NOTWRITABLE);
+	    netsnmp_request_set_error(request, SNMP_ERR_COMMITFAILED);
 	    goto done;
 	}
 	break;
@@ -772,6 +787,7 @@ clixon_snmp_scalar_handler1(netsnmp_mib_handler          *handler,
  ok:
     retval = SNMP_ERR_NOERROR;
  done:
+    clicon_debug(1, "%s retval:%d", __FUNCTION__, retval);
     return retval;
 }
 
@@ -1307,7 +1323,6 @@ clixon_snmp_table_handler1(netsnmp_mib_handler          *handler,
 				      reqinfo, request)) < 0)
 	    goto done;
 	if (ret == 0){
-	    //	    if ((ret = netsnmp_request_set_error(request, SNMP_NOSUCHOBJECT)) != SNMPERR_SUCCESS){
 	    if ((ret = netsnmp_request_set_error(request, SNMP_NOSUCHOBJECT)) != SNMPERR_SUCCESS){
 
 		clicon_err(OE_SNMP, ret, "netsnmp_request_set_error");
@@ -1318,7 +1333,7 @@ clixon_snmp_table_handler1(netsnmp_mib_handler          *handler,
         break;
     case MODE_SET_RESERVE1: // 0
 	if (!yang_config_ancestor(sh->sh_ys)){
-	    retval = SNMP_ERR_NOTWRITABLE;
+	    netsnmp_request_set_error(request, SNMP_ERR_NOTWRITABLE);
 	    goto done;;	    
 	}
 	// Check types: compare type in requestvb to yang type (or do later)
@@ -1337,6 +1352,18 @@ clixon_snmp_table_handler1(netsnmp_mib_handler          *handler,
 	    }
 	    clicon_debug(1, "%s Nosuchinstance", __FUNCTION__);
 	}
+	/*
+	 * There does not seem to be a separate validation action and commit does not 
+	 * return an error.
+	 * Therefore validation is done here directly as well as discard if it fails.
+	 */
+	if ((ret = clicon_rpc_validate(sh->sh_h, "candidate")) < 0)
+	    goto done;
+	if (ret == 0){
+	    clicon_rpc_discard_changes(sh->sh_h);
+	    netsnmp_request_set_error(request, SNMP_ERR_COMMITFAILED);
+	    goto done;
+	}
 	break;
     case MODE_SET_COMMIT:   // 3
 	if ((ret = clicon_rpc_commit(sh->sh_h, 0, 0, 0, NULL, NULL)) < 0)

apps/snmp/snmp_main.c

@@ -309,7 +309,7 @@ usage(clicon_handle h,
     fprintf(stderr, "usage:%s\n"
 	    "where options are\n"
             "\t-h\t\tHelp\n"
-	    "\t-D <level>\tDebug level\n"
+	    "\t-D <level>\tDebug level (>1 for extensive libnetsnmp debug)\n"
     	    "\t-f <file>\tConfiguration file (mandatory)\n"
 	    "\t-l (e|o|s|f<file>) Log on std(e)rr, std(o)ut, (s)yslog(default), (f)ile\n"
     	    "\t-z\t\tKill other %s daemon and exit\n"
@@ -384,7 +384,9 @@ main(int    argc,
      */
     clicon_log_init(__PROGRAM__, dbg?LOG_DEBUG:LOG_INFO, logdst); 
     clicon_debug_init(dbg, NULL); 
-
+    /* This is netsnmplib debugging which is quite extensive + only if compiled w debug */
+    if (dbg > 1)
+    	snmp_set_do_debugging(1);
     /*
      * Register error category and error/log callbacks for netsnmp special error handling
      */

test/test_snmp_set.sh

@@ -276,7 +276,7 @@ new "wait backend"
 wait_backend
 
 new "set value with error"
-expectpart "$($snmpset ${MIB}.1.1  i 4321)" 0 "INTEGER: 4321"
+expectpart "$($snmpset ${MIB}.1.1  i 4321 2>&1)" 2 "commitFailed"
 
 new "Cleaning up"
 testexit

test/test_snmp_system.sh

@@ -107,12 +107,14 @@ function testinit(){
     wait_backend
 
     if [ $SN -ne 0 ]; then
-    # Kill old clixon_snmp, if any
+	# Kill old clixon_snmp, if any
-    new "Terminating any old clixon_snmp processes"
+	new "Terminating any old clixon_snmp processes"
-    sudo killall -q clixon_snmp
+	sudo killall -q clixon_snmp
 	
-    new "Starting clixon_snmp"
+	new "Starting clixon_snmp"
-    start_snmp $cfg &
+	# XXX augmented objects seem to be registered twice: error: duplicate registration: MIB modules snmpSetSerialNo and AgentX subagent 52, session 0x562087a70e20, subsession 0x562087a820c0 (oid .1.3.6.1.6.3.1.1.6.1).
+
+	start_snmp $cfg &
     fi
 
     new "wait snmp"