From 05b31508a10a953423fdaa1187a11414690e61f9 Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Tue, 25 Oct 2022 16:32:48 +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) * Fixed by validating writes on ACTION instead of COMMIT since libnetsnmp seems not to accept commit errors --- CHANGELOG.md | 2 ++ apps/snmp/snmp_handler.c | 37 ++++++++++++++++++++++++++++++++----- apps/snmp/snmp_main.c | 6 ++++-- test/test_snmp_set.sh | 2 +- test/test_snmp_system.sh | 12 +++++++----- 5 files changed, 46 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e9d23ee7..da8a4225 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/apps/snmp/snmp_handler.c b/apps/snmp/snmp_handler.c index 216bb753..1917b4d4 100644 --- a/apps/snmp/snmp_handler.c +++ b/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; - goto done;; + netsnmp_request_set_error(request, SNMP_ERR_NOTWRITABLE); + 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) diff --git a/apps/snmp/snmp_main.c b/apps/snmp/snmp_main.c index 8c41e1cd..0e548636 100644 --- a/apps/snmp/snmp_main.c +++ b/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 \tDebug level\n" + "\t-D \tDebug level (>1 for extensive libnetsnmp debug)\n" "\t-f \tConfiguration file (mandatory)\n" "\t-l (e|o|s|f) 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 */ diff --git a/test/test_snmp_set.sh b/test/test_snmp_set.sh index 9907941d..8bdcd7c5 100755 --- a/test/test_snmp_set.sh +++ b/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 diff --git a/test/test_snmp_system.sh b/test/test_snmp_system.sh index bbee27de..50618522 100755 --- a/test/test_snmp_system.sh +++ b/test/test_snmp_system.sh @@ -107,12 +107,14 @@ function testinit(){ wait_backend if [ $SN -ne 0 ]; then - # Kill old clixon_snmp, if any - new "Terminating any old clixon_snmp processes" - sudo killall -q clixon_snmp + # Kill old clixon_snmp, if any + new "Terminating any old clixon_snmp processes" + sudo killall -q clixon_snmp + + new "Starting clixon_snmp" + # 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). - new "Starting clixon_snmp" - start_snmp $cfg & + start_snmp $cfg & fi new "wait snmp"