SNMP Frontend, fix handling of snmpd down and memory leaks

If snmpd is down, clixon_snmp does not start
If snmpd stops, clixon_snmp quits
Mem leaks fixed

apps/snmp/snmp_handler.c

@@ -340,6 +340,8 @@ snmp_scalar_set(clicon_handle               h,
  ok:
     retval = 0;
  done:
+    if (api_path)
+	free(api_path);
     if (cb)
 	cbuf_free(cb);
     if (xtop)

apps/snmp/snmp_lib.c

@@ -139,15 +139,51 @@ snmp_msg_int2str(int msg)
     return clicon_int2str(snmp_msg_map, msg);
 }
 
-/*! Free clixon snmp handler struct
+/*! Duplicate clixon snmp handler struct
+ * Use signature of libnetsnmp data_clone field of netsnmp_mib_handler in agent_handler.h
+ * @param[in]  arg
  */
-int
+void*
-snmp_handle_free(clixon_snmp_handle *sh)
+snmp_handle_clone(void *arg)
 {
+    clixon_snmp_handle *sh0 = (clixon_snmp_handle *)arg;
+    clixon_snmp_handle *sh1 = NULL;
+
+    if (sh0 == NULL)
+	return NULL;
+    if ((sh1 = malloc(sizeof(*sh1))) == NULL){
+       clicon_err(OE_UNIX, errno, "malloc");
+       return NULL;
+    }
+    memset(sh1, 0, sizeof(*sh1));
+    if (sh0->sh_cvk &&
+	(sh1->sh_cvk = cvec_dup(sh0->sh_cvk)) == NULL){
+	clicon_err(OE_UNIX, errno, "cvec_dup");
+	return NULL;
+    }
+    return (void*)sh1;
+}
+
+/*! Free clixon snmp handler struct
+ * Use signature of libnetsnmp data_free field of netsnmp_mib_handler in agent_handler.h
+ * @param[in]  arg
+ */
+void
+snmp_handle_free(void *arg)
+{
+    clixon_snmp_handle *sh = (clixon_snmp_handle *)arg;
+
+    if (sh != NULL){
 	if (sh->sh_cvk)
 	    cvec_free(sh->sh_cvk);
+	if (sh->sh_table_info){
+	    if (sh->sh_table_info->indexes){
+		snmp_free_varbind(sh->sh_table_info->indexes);
+	    }
+	    free(sh->sh_table_info);
+	}
 	free(sh);
-    return 0;
+    }
 }
 
 /*! Translate from YANG to SNMP asn1.1 type ids (not value)
@@ -169,7 +205,7 @@ type_yang2asn1(yang_stmt    *ys,
     int        retval = -1;
     yang_stmt *yrestype;        /* resolved type */
     char      *restype;         /* resolved type */
-    char      *origtype=NULL;   /* original type */
+    char      *origtype = NULL; /* original type */
     int        at;
     yang_stmt *ypath;
     yang_stmt *yref;
@@ -186,6 +222,10 @@ type_yang2asn1(yang_stmt    *ys,
 	}
 	if (yang_path_arg(ys, yang_argument_get(ypath), &yref) < 0)
 	    goto done;
+	if (origtype){
+	    free(origtype);
+	    origtype = NULL;
+	}
 	if (yang_type_get(yref, &origtype, &yrestype, NULL, NULL, NULL, NULL, NULL) < 0)
 	    goto done;
 	restype = yrestype?yang_argument_get(yrestype):NULL;
@@ -226,6 +266,8 @@ type_yang2asn1(yang_stmt    *ys,
 	*asn1_type = at;
     retval = 0;
  done:
+    if (origtype)
+	free(origtype);
     return retval;
 }
 
@@ -251,10 +293,10 @@ type_snmp2xml(yang_stmt                  *ys,
     int          retval = -1;
     char        *cvstr;
     enum cv_type cvtype;
-    cg_var      *cv;
+    cg_var      *cv = NULL;
     yang_stmt   *yrestype;        /* resolved type */
     char        *restype;         /* resolved type */
-    char        *origtype=NULL;   /* original type */
+    char        *origtype = NULL; /* original type */
 
     clicon_debug(1, "%s", __FUNCTION__);
     if (valstr == NULL){
@@ -326,14 +368,17 @@ type_snmp2xml(yang_stmt                  *ys,
 	goto fail;
 	break;
     }
-      
     if ((*valstr = cv2str_dup(cv)) == NULL){
 	clicon_err(OE_UNIX, errno, "cv2str_dup");
 	goto done;
     }
     retval = 1;
  done:
-    clicon_debug(1, "%s %d", __FUNCTION__, retval);
+    clicon_debug(2, "%s %d", __FUNCTION__, retval);
+    if (cv)
+	cv_free(cv);
+    if (origtype)
+	free(origtype);
     return retval;
  fail:
     retval = 0;
@@ -361,7 +406,7 @@ type_xml2snmp_pre(char      *xmlstr0,
     int        retval = -1;
     yang_stmt *yrestype;        /* resolved type */
     char      *restype;         /* resolved type */
-    char      *origtype=NULL;   /* original type */
+    char      *origtype = NULL; /* original type */
     char      *str = NULL;
     int        ret;
 
@@ -402,6 +447,8 @@ type_xml2snmp_pre(char      *xmlstr0,
     retval = 1;
  done:
     clicon_debug(2, "%s %d", __FUNCTION__, retval);
+    if (origtype)
+	free(origtype);
     return retval;
  fail:
     retval = 0;
@@ -693,3 +740,34 @@ snmp_body2oid(cxobj  *xi,
 	cbuf_free(enc);
     return retval;
 }
+
+/*========== libnetsnmp-specific code ===============
+ * Peeks into internal lib global variables, may be sensitive to library change
+ */
+/*! Check if netsnmp is connected 
+ * @retval 1 yes, running
+ * @retval 0 No, not running
+ * XXX: this peeks into the "main_session" global variable in agent/snmp_agent.c
+ *      Tried to find API function but failed
+ */
+int
+snmp_agent_check(void)
+{
+    extern netsnmp_session *main_session;
+    
+    return (main_session != NULL) ? 1 : 0;
+}
+
+/*! Cleanup remaining libnetsnmb memory
+ * XXX: this peeks into the "tclist" global variable in snmplib/parse.c
+ *      Tried to find API function but failed
+ */
+int
+snmp_agent_cleanup(void)
+{
+    extern void *tclist;
+    
+    if (tclist)
+	free(tclist);
+    return 0;
+}

apps/snmp/snmp_lib.h

@@ -52,6 +52,7 @@ struct clixon_snmp_handle {
     size_t        sh_oidlen;           
     char         *sh_default;          /* MIB default value leaf only */
     cvec         *sh_cvk;              /* Index/Key variables */
+    netsnmp_table_registration_info *sh_table_info; /* To mimic table-handler in libnetsnmp code*/
 };
 typedef struct clixon_snmp_handle clixon_snmp_handle;
 
@@ -60,7 +61,8 @@ typedef struct clixon_snmp_handle clixon_snmp_handle;
  */
 int   snmp_access_str2int(char *modes_str);
 const char *snmp_msg_int2str(int msg);
-int   snmp_handle_free(clixon_snmp_handle *sh);
+void  *snmp_handle_clone(void *arg);
+void   snmp_handle_free(void *arg);
 int   type_yang2asn1(yang_stmt *ys, int *asn1_type, int extended);
 int   type_snmp2xml(yang_stmt                  *ys,
 		    netsnmp_variable_list      *requestvb,
@@ -71,6 +73,8 @@ int   type_xml2snmp_pre(char *xmlstr, yang_stmt *ys, char **snmpstr);
 int   type_xml2snmp(char *snmpstr, int *asn1type, u_char **snmpval, size_t *snmplen, char **reason);
 int   yang2xpath(yang_stmt *ys, cvec *keyvec, char **xpath);
 int   snmp_body2oid(cxobj  *xi, cg_var *cv);
+int   snmp_agent_check(void);
+int   snmp_agent_cleanup(void);
 
 #endif /* _SNMP_LIB_H_ */
 

apps/snmp/snmp_main.c

@@ -30,6 +30,13 @@
   the terms of any one of the Apache License version 2 or the GPL.
 
   ***** END LICENSE BLOCK *****
+  * This is the clixon_snmp daemon
+  * It assumes a netsnmp damon is running. 
+  * - If netsnmp does not run, clixon_snmp will not start
+  * - If netsnmp dies, clixon_snmp will exit
+  * - If netsnmp is restarted, so should clixon_snmp be
+  * It is possible to be more resilient, such as setting a timer and trying again, in fact, libnetsnmp
+  * has some such mechanisms
  */
 
 #ifdef HAVE_CONFIG_H
@@ -56,11 +63,15 @@
 /* clicon */
 #include <clixon/clixon.h>
 
+#include "snmp_lib.h"
 #include "snmp_register.h"
 
 /* Command line options to be passed to getopt(3) */
 #define SNMP_OPTS "hD:f:l:o:z"
 
+/* Forward */
+static int clixon_snmp_input_cb(int s, void *arg);
+    
 /*! Return (hardcoded) pid file
  */
 static char*
@@ -84,6 +95,87 @@ clixon_snmp_sig_term(int arg)
     clixon_exit_set(1); 
 }
 
+/*! Clean and close all state of netconf process (but dont exit). 
+ * Cannot use h after this 
+ * @param[in]  h  Clixon handle
+ */
+static int
+snmp_terminate(clicon_handle h)
+{
+    yang_stmt *yspec;
+    cvec      *nsctx;
+    cxobj     *x;
+    char      *pidfile = clicon_snmp_pidfile(h);
+
+    snmp_shutdown(__FUNCTION__);
+    shutdown_agent();
+    snmp_agent_cleanup();
+    clicon_rpc_close_session(h);
+    if ((yspec = clicon_dbspec_yang(h)) != NULL)
+	ys_free(yspec);
+    if ((yspec = clicon_config_yang(h)) != NULL)
+	ys_free(yspec);
+    if ((nsctx = clicon_nsctx_global_get(h)) != NULL)
+	cvec_free(nsctx);
+    if ((x = clicon_conf_xml(h)) != NULL)
+	xml_free(x);
+    xpath_optimize_exit();
+    clixon_event_exit();
+    clicon_handle_exit(h);
+    clixon_err_exit();
+    clicon_log_exit();
+    if (pidfile)
+	unlink(pidfile);
+    return 0;
+}
+
+/*! Get which sockets are used from SNMP API, the register single sockets into clixon event system
+ *
+ * @param[in]  h        Clixon handle
+ * @param[in]  register if 1 register snmp sockets with event handler. If 0 close and unregister
+ * This is a workaround for netsnmps API usiing fdset:s, instead an fdset is created before calling
+ * the snmp api
+ * if you use select(), see snmp_select_info() in snmp_api(3) 
+ * snmp_select_info(int *numfds, fd_set *fdset, struct timeval *timeout, int *block)
+ * @see clixon_snmp_input_cb
+ */
+static int
+clixon_snmp_fdset_register(clicon_handle h,
+			   int           regfd)
+{
+    int             retval = -1;
+    int             numfds = 0;
+    fd_set          readfds;
+    struct timeval  timeout = { LONG_MAX, 0 };
+    int             block = 0;
+    int             nr;
+    int             s;
+
+    FD_ZERO(&readfds);
+    if ((nr = snmp_sess_select_info(NULL, &numfds, &readfds, &timeout, &block)) < 0){
+	clicon_err(OE_XML, errno, "snmp_select_error");
+	goto done;
+    }
+    /* eg 4, 6, 8 */
+    for (s=0; s<numfds; s++){
+	if (FD_ISSET(s, &readfds)){
+	    clicon_debug(1, "%s %d", __FUNCTION__, s);
+	    if (regfd){
+		if (clixon_event_reg_fd(s, clixon_snmp_input_cb, h, "snmp socket") < 0)
+		    goto done;
+	    }
+	    else{
+		if (clixon_event_unreg_fd(s, clixon_snmp_input_cb) < 0)
+		    goto done;
+		close(s);
+	    }
+	}
+    }
+    retval = 0;
+ done:
+    return retval;
+}
+
 /*! Callback for single socket 
  * This is a workaround for netsnmps API usiing fdset:s, instead an fdset is created before calling
  * the snmp api
@@ -96,44 +188,34 @@ clixon_snmp_input_cb(int   s,
 {
     int            retval = -1;
     fd_set         readfds;
-    //    clicon_handle h = (clicon_handle)arg;
+    clicon_handle  h = (clicon_handle)arg;
+    int            ret;
 
-    clicon_debug(2, "%s", __FUNCTION__);
+    clicon_debug(1, "%s %d", __FUNCTION__, s);
     FD_ZERO(&readfds);
     FD_SET(s, &readfds);
-    snmp_read(&readfds);
+    (void)snmp_read(&readfds);
-    retval = 0;
+    if (clixon_event_poll(s) < 0){
-    // done:
+	if (errno == EBADF){
-    return retval;
+	    clicon_err_reset();
-}
+	    /* Close the active socket */
-
+	    if (clixon_event_unreg_fd(s, clixon_snmp_input_cb) < 0)
-/*! Get which sockets are used from SNMP API, the register single sockets into clixon event system
+		goto done;
- *
+	    close(s);
- * This is a workaround for netsnmps API usiing fdset:s, instead an fdset is created before calling
+	    /* and then the others */
- * the snmp api
+	    if (clixon_snmp_fdset_register(h, 0) < 0)
- * if you use select(), see snmp_select_info() in snmp_api(3) 
+		goto done;
- * snmp_select_info(int *numfds, fd_set *fdset, struct timeval *timeout, int *block)
+	    if ((ret = snmp_close_sessions()) != 1){
- * @see clixon_snmp_input_cb
+		clicon_err(OE_SNMP, ret, "snmp_close_sessions");
- */
-static int
-clixon_snmp_fdset_register(clicon_handle h)
-{
-    int             retval = -1;
-    int             numfds = 0;
-    fd_set          readfds;
-    struct timeval  timeout = { LONG_MAX, 0 };
-    int             block = 0;
-    int             nr;
-    int             i;
-
-    FD_ZERO(&readfds);
-    if ((nr = snmp_sess_select_info(NULL, &numfds, &readfds, &timeout, &block)) < 0){
-	clicon_err(OE_XML, errno, "snmp_select_error");
 		goto done;
 	    }
-    for (i=0; i<numfds; i++){
+	    /* Signal normal exit to upper layers (=event handling)
-	if (FD_ISSET(i, &readfds)){
+	     * One can signal error and return -1, but it is nicer with an orderly exit
-	    if (clixon_event_reg_fd(i, clixon_snmp_input_cb, h, "snmp socket") < 0)
+	     */
+	    clixon_exit_set(1);
+	}
+	else {
+	    clicon_err(OE_UNIX, errno, "poll");
 	    goto done;
 	}
     }
@@ -142,13 +224,14 @@ clixon_snmp_fdset_register(clicon_handle h)
     return retval;
 }
 
+
 /*! Init netsnmp agent connection
  * @param[in]  h      Clixon handle
  * @param[in]  logdst Log destination, see clixon_log.h
  * @see snmp_terminate
  */
 static int
-clixon_snmp_subagent(clicon_handle h,
+clixon_snmp_init_subagent(clicon_handle h,
 			  int           logdst)
 {
     int   retval = -1;
@@ -159,8 +242,17 @@ clixon_snmp_subagent(clicon_handle h,
 	snmp_enable_calllog();
     else
 	snmp_enable_stderrlog();
-    /* make a agentx client. */
+    /* 0 if master, 1 if client */
     netsnmp_ds_set_boolean(NETSNMP_DS_APPLICATION_ID, NETSNMP_DS_AGENT_ROLE, 1);
+    /* don't load config and don't load/save persistent file */
+    netsnmp_ds_set_boolean(NETSNMP_DS_LIBRARY_ID, NETSNMP_DS_LIB_DONT_PERSIST_STATE, 1);
+    /* don't load persistent file */
+    netsnmp_ds_set_boolean(NETSNMP_DS_LIBRARY_ID, NETSNMP_DS_LIB_DISABLE_PERSISTENT_LOAD, 1);    
+    /* don't save persistent file */
+    netsnmp_ds_set_boolean(NETSNMP_DS_LIBRARY_ID, NETSNMP_DS_LIB_DISABLE_PERSISTENT_SAVE, 1);    
+
+    if (clicon_debug_get())
+	netsnmp_ds_set_boolean(NETSNMP_DS_APPLICATION_ID, NETSNMP_DS_AGENT_VERBOSE, 1);
 
     if ((sockpath = clicon_option_str(h, "CLICON_SNMP_AGENT_SOCK")) == NULL){
 	clicon_err(OE_XML, 0, "CLICON_SNMP_AGENT_SOCK not set");
@@ -169,13 +261,17 @@ clixon_snmp_subagent(clicon_handle h,
     /* XXX: This should be configurable. */
     netsnmp_ds_set_string(NETSNMP_DS_APPLICATION_ID, NETSNMP_DS_AGENT_X_SOCKET, sockpath);
 
-
     /* initialize the agent library */
     init_agent(__PROGRAM__);
 
     /* example-demon will be used to read example-demon.conf files. */
     init_snmp(__PROGRAM__);
 
+    if (!snmp_agent_check()){
+	clicon_err(OE_DAEMON, 0, "Connection to SNMP agent failed");
+	goto done;
+    }
+	
     if (set_signal(SIGTERM, clixon_snmp_sig_term, NULL) < 0){
 	clicon_err(OE_DAEMON, errno, "Setting signal");
 	goto done;
@@ -189,7 +285,7 @@ clixon_snmp_subagent(clicon_handle h,
 	goto done;
     }
    /* Workaround for netsnmps API use of fdset:s instead of sockets */
-    if (clixon_snmp_fdset_register(h) < 0)
+    if (clixon_snmp_fdset_register(h, 1) < 0)
 	goto done;
     retval = 0;
  done:
@@ -226,37 +322,6 @@ clixon_snmp_err_cb(void *handle,
     return 0;
 }
 
-/*! Clean and close all state of netconf process (but dont exit). 
- * Cannot use h after this 
- * @param[in]  h  Clixon handle
- */
-static int
-snmp_terminate(clicon_handle h)
-{
-    yang_stmt  *yspec;
-    cvec       *nsctx;
-    cxobj      *x;
-    char      *pidfile = clicon_snmp_pidfile(h);
-    
-    shutdown_agent();
-    clicon_rpc_close_session(h);
-    if ((yspec = clicon_dbspec_yang(h)) != NULL)
-	ys_free(yspec);
-    if ((yspec = clicon_config_yang(h)) != NULL)
-	ys_free(yspec);
-    if ((nsctx = clicon_nsctx_global_get(h)) != NULL)
-	cvec_free(nsctx);
-    if ((x = clicon_conf_xml(h)) != NULL)
-	xml_free(x);
-    xpath_optimize_exit();
-    clixon_event_exit();
-    clicon_handle_exit(h);
-    clixon_err_exit();
-    clicon_log_exit();
-    if (pidfile)
-	unlink(pidfile);
-    return 0;
-}
 
 /*! Usage help routine
  * @param[in]  h      Clixon handle
@@ -487,17 +552,14 @@ main(int    argc,
 	goto done;
     clicon_session_id_set(h, id);
     
-
-    
     /* Init snmp as subagent */
-    if (clixon_snmp_subagent(h, logdst) < 0)
+    if (clixon_snmp_init_subagent(h, logdst) < 0)
 	goto done;
 
     /* Init and traverse mib-translated yangs and register callbacks */
     if (clixon_snmp_traverse_mibyangs(h) < 0)
 	goto done;
 
-
     /* Write pid-file */
     if (pidfile_write(pidfile) <  0)
 	goto done;

apps/snmp/snmp_register.c

@@ -182,7 +182,11 @@ mibyang_leaf_register(clicon_handle h,
 	netsnmp_handler_free(handler);
 	goto done;
     }
-    handler->myvoid =(void*)sh;
+    /* Register our application data and how to free it */
+    handler->myvoid = (void*)sh;
+    handler->data_clone = snmp_handle_clone;
+    handler->data_free = snmp_handle_free;
+
     /* 
      * XXX: nhreg->agent_data
      */
@@ -231,7 +235,7 @@ mibyang_table_register(clicon_handle h,
     clixon_snmp_handle              *sh;
     int                              ret;
     netsnmp_mib_handler             *handler;
-    netsnmp_table_registration_info *table_info=NULL;
+    netsnmp_table_registration_info *table_info = NULL;
     cvec                            *cvk = NULL; /* vector of index keys */
     cg_var                          *cvi;
     char                            *keyname;
@@ -274,7 +278,11 @@ mibyang_table_register(clicon_handle h,
 	netsnmp_handler_free(handler);
 	goto done;
     }
+    /* Register our application data and how to free it */
     handler->myvoid =(void*)sh;
+    handler->data_clone = snmp_handle_clone;
+    handler->data_free = snmp_handle_free;
+    
     /* See netsnmp_register_table_data_set */
     if ((table_info = SNMP_MALLOC_TYPEDEF(netsnmp_table_registration_info)) == NULL){
 	clicon_err(OE_UNIX, errno, "SNMP_MALLOC_TYPEDEF");
@@ -320,6 +328,7 @@ mibyang_table_register(clicon_handle h,
 	clicon_err(OE_SNMP, ret, "netsnmp_register_table");
 	goto done;
     }
+    sh->sh_table_info = table_info;
     clicon_debug(1, "%s %s registered", __FUNCTION__, oidstr);
  ok:
     retval = 0;
@@ -346,7 +355,7 @@ mibyang_table_traverse_static(clicon_handle h,
 {
     int        retval = -1;
     cvec      *nsc = NULL;
-    char      *xpath;
+    char      *xpath = NULL;
     cxobj     *xt = NULL;
     cxobj     *xerr;
     cxobj     *xtable;
@@ -408,6 +417,8 @@ mibyang_table_traverse_static(clicon_handle h,
     }
     retval = 0;
  done:
+    if (xpath)
+	free(xpath);
     if (cvk)
         cvec_free(cvk);
     if (xt)

test/lib.sh

@@ -190,6 +190,9 @@ BUSER=clicon
 
 : ${clixon_snmp_pidfile:="/var/tmp/clixon_snmp.pid"}
 
+# Temporary debug var, set to trigger remaining snmp errors
+: ${snmp_debug:=false}
+
 # Source the site-specific definitions for test script variables, if site.sh
 # exists. The variables defined in site.sh override any variables of the same
 # names in the environment in the current execution.

test/test_snmp_get.sh

@@ -250,7 +250,8 @@ validate_oid $NAME12 $NAME12 "INTEGER" 1
 new "Get bulk OIDs"
 expectpart "$($snmpbulkget $OID1)" 0 "$OID2 = INTEGER: -1" "$OID3 = STRING: \"This is not default\"" "$OID4 = Timeticks: (12345) 0:02:03.45" "$OID5 = INTEGER: 48" "$OID6 = Gauge32: 123123123" "$OID7 = INTEGER: 3" "$OID8 = Counter32: 123456" "$OID9 = Counter64: 4294967296" "$OID10 = INTEGER: 1" "$OID11 = Timeticks: (1234567890) 142 days, 21:21:18.90"
 
-if [ $snmp_debug ]; then
+snmp_debug=false
+if $snmp_debug; then
     new "Test SNMP table netSnmpIETFWGTable"
     expectpart "$($snmptable $OID13)" 0 "Name1" "Name2"