rtpengine: Fix comments for hastable

- shm NULL checks and free already alloc'ed shm
- default entry tout to 3600 sec
- return node only, not the whole entry
- zero shm hashtable parts
- lookup and select new node if lookup fails; this is done for all commands
and assures fallback behaviour
- change void to struct specific
- make set_rtp_inst_pvar() static -> used only in rtpengine.c
- fix typos rtpproxy vs rtpengine

modules/rtpengine/doc/rtpengine_admin.xml

@@ -361,7 +361,7 @@ modparam("rtpengine", "read_sdp_pv", "$var(sdp)")
 route {
 	...
 	$var(sdp) = $rb + "a=foo:bar\r\n";
-	rtpproxy_manage();
+	rtpengine_manage();
 }
 </programlisting>
 		</example>
@@ -386,7 +386,7 @@ modparam("rtpengine", "write_sdp_pv", "$avp(sdp)")
 ...
 route {
 	...
-	rtpproxy_manage();
+	rtpengine_manage();
 	set_body("$avp(sdp)a=baz123\r\n", "application/sdp");
 }
 </programlisting>
@@ -406,7 +406,7 @@ route {
 		<title>Set <varname>rtp_inst_pvar</varname> parameter</title>
 <programlisting format="linespecific">
 ...
-modparam("rtpproxy", "rtp_inst_pvar", "$avp(RTP_INSTANCE)")
+modparam("rtpengine", "rtp_inst_pvar", "$avp(RTP_INSTANCE)")
 ...
 </programlisting>
 		</example>
@@ -424,7 +424,7 @@ modparam("rtpproxy", "rtp_inst_pvar", "$avp(RTP_INSTANCE)")
 		<title>Set <varname>hash_table_size</varname> parameter</title>
 <programlisting format="linespecific">
 ...
-modparam("rtpproxy", "hash_table_size", "123")
+modparam("rtpengine", "hash_table_size", "123")
 ...
 </programlisting>
 		</example>
@@ -434,23 +434,24 @@ modparam("rtpproxy", "hash_table_size", "123")
 		<title><varname>hash_table_tout</varname> (integer)</title>
 		<para>
 			Number of seconds after an rtpengine hash table entry is marked for deletion.
-			By default, this parameter is set to 120 (seconds).
+			By default, this parameter is set to 3600 (seconds).
 		</para>
 		<para>
 			To maintain information about a selected rtp machine node, for a given call, entries are added in a hashtable of (callid, node) pairs.
-			When "offer" comes, insert new entry in the hastable.
-			When subsequent commands come, lookup callid and return chosen node.
-			When "delete" comes, remove old entry from hashtable.
+			When command comes, lookup callid. If found, return chosen node. If not found, choose a new node, insert it in the hastable and return the chosen node.
 		</para>
 		<para>
 			NOTE: In the current implementation, the actual deletion happens <emphasis>on the fly</emphasis>,
 			while insert/remove/lookup the hastable, <emphasis>only</emphasis> for the entries in the insert/remove/lookup path.
 		</para>
+		<para>
+			NOTE: When configuring this parameter, one should consider maximum call time VS share memory for unfinished calls.
+		</para>
 		<example>
 		<title>Set <varname>hash_table_tout</varname> parameter</title>
 <programlisting format="linespecific">
 ...
-modparam("rtpproxy", "hash_table_tout", "300")
+modparam("rtpengine", "hash_table_tout", "300")
 ...
 </programlisting>
 		</example>

modules/rtpengine/rtpengine.c

@@ -218,6 +218,7 @@ static int rtpp_test_ping(struct rtpp_node *node);
 
 /* Pseudo-Variables */
 static int pv_get_rtpstat_f(struct sip_msg *, pv_param_t *, pv_value_t *);
+static int set_rtp_inst_pvar(struct sip_msg *msg, const str * const uri);
 
 /*mi commands*/
 static struct mi_root* mi_enable_rtp_proxy(struct mi_root* cmd_tree, void* param);
@@ -235,7 +236,7 @@ static pid_t mypid;
 static unsigned int myseqn = 0;
 static str extra_id_pv_param = {NULL, 0};
 static char *setid_avp_param = NULL;
-static int hash_table_tout = 120;
+static int hash_table_tout = 3600;
 static int hash_table_size = 256;
 
 static char ** rtpp_strings=0;
@@ -2284,6 +2285,7 @@ select_rtpp_node_new(str callid, int do_test, int op)
 	int was_forced = 0;
 
 	/* XXX Use quick-and-dirty hashing algo */
+	sum = 0;
 	for(i = 0; i < callid.len; i++)
 		sum += callid.s[i];
 	sum &= 0xff;
@@ -2347,26 +2349,38 @@ found:
 			goto retry;
 	}
 
-	/* build hash table entry */
+	/* build the entry */
 	struct rtpengine_hash_entry *entry = shm_malloc(sizeof(struct rtpp_node));
+	if (!entry) {
+		LM_ERR("rtpengine hash table fail to create entry for calllen=%d callid=%.*s\n",
+			callid.len, callid.len, callid.s);
+		return node;
+	}
+
+	/* fill the entry */
 	if (shm_str_dup(&entry->callid, &callid) < 0) {
-		LM_ERR("rtpengine hash table fail to duplicate calllen=%d callid=%.*s",
+		LM_ERR("rtpengine hash table fail to duplicate calllen=%d callid=%.*s\n",
 			callid.len, callid.len, callid.s);
+		shm_free(entry);
+		return node;
 	}
 	entry->node = node;
 	entry->next = NULL;
 	entry->tout = get_ticks() + hash_table_tout;
 
-	/* Insert the key<->entry from the hashtable */
+	/* insert the key<->entry from the hashtable */
 	if (!rtpengine_hash_table_insert(&callid, entry)) {
-		LM_ERR("rtpengine hash table fail to insert node=%.*s for calllen=%d callid=%.*s",
+		LM_ERR("rtpengine hash table fail to insert node=%.*s for calllen=%d callid=%.*s\n",
 			node->rn_url.len, node->rn_url.s, callid.len, callid.len, callid.s);
+		shm_free(entry->callid.s);
+		shm_free(entry);
+		return node;
 	} else {
 		LM_DBG("rtpengine hash table insert node=%.*s for calllen=%d callid=%.*s\n",
 			node->rn_url.len, node->rn_url.s, callid.len, callid.len, callid.s);
 	}
 
-	/* Return selected node  */
+	/* return selected node */
 	return node;
 }
 
@@ -2377,20 +2391,11 @@ static struct rtpp_node *
 select_rtpp_node_old(str callid, int do_test, int op)
 {
 	struct rtpp_node *node = NULL;
-	struct rtpengine_hash_entry *entry = NULL;
 
-	entry = rtpengine_hash_table_lookup(&callid);
-	if (!entry) {
-		LM_ERR("rtpengine hash table lookup failed to find entry for calllen=%d callid=%.*s\n",
-			callid.len, callid.len, callid.s);
-	} else {
-		LM_DBG("rtpengine hash table lookup find entry for calllen=%d callid=%.*s\n",
-			callid.len, callid.len, callid.s);
-		node = entry->node;
-	}
+	node = rtpengine_hash_table_lookup(&callid);
 
 	if (!node) {
-		LM_ERR("rtpengine hash table lookup failed to find node for calllen=%d callid=%.*s\n",
+		LM_NOTICE("rtpengine hash table lookup failed to find node for calllen=%d callid=%.*s\n",
 			callid.len, callid.len, callid.s);
 		return NULL;
 	} else {
@@ -2398,6 +2403,39 @@ select_rtpp_node_old(str callid, int do_test, int op)
 			node->rn_url.len, node->rn_url.s, callid.len, callid.len, callid.s);
 	}
 
+	return node;
+}
+
+/*
+ * Main balancing routine. This DO try to keep the same proxy for
+ * the call if some proxies were disabled or enabled (e.g. kamctl command)
+ */
+static struct rtpp_node *
+select_rtpp_node(str callid, int do_test, int op)
+{
+	struct rtpp_node *node = NULL;
+
+	if(!active_rtpp_set) {
+		LM_ERR("script error - no valid set selected\n");
+		return NULL;
+	}
+
+	// lookup node
+	node = select_rtpp_node_old(callid, do_test, op);
+
+	// check node
+	if (!node) {
+		// run the selection algorithm
+		node = select_rtpp_node_new(callid, do_test, op);
+
+		// check node
+		if (!node) {
+			LM_ERR("rtpengine failed to select new for calllen=%d callid=%.*s\n",
+				callid.len, callid.len, callid.s);
+			return NULL;
+		}
+	}
+
 	// if node enabled, return it
 	if (!node->rn_disabled) {
 		return node;
@@ -2420,28 +2458,6 @@ select_rtpp_node_old(str callid, int do_test, int op)
 	return NULL;
 }
 
-/*
- * Main balancing routine. This DO try to keep the same proxy for
- * the call if some proxies were disabled or enabled (e.g. kamctl command)
- */
-static struct rtpp_node *
-select_rtpp_node(str callid, int do_test, int op)
-{
-	if(!active_rtpp_set) {
-		LM_ERR("script error - no valid set selected\n");
-		return NULL;
-	}
-
-	// calculate and choose a node
-	if (op == OP_OFFER) {
-		// run the selection algorithm
-		return select_rtpp_node_new(callid, do_test, op);
-	} else {
-		// lookup the hastable (key=callid value=node) and get the old node
-		return select_rtpp_node_old(callid, do_test, op);
-	}
-}
-
 static int
 get_extra_id(struct sip_msg* msg, str *id_str) {
 	if(msg==NULL || extra_id_pv==NULL || id_str==NULL) {
@@ -2858,7 +2874,8 @@ pv_get_rtpstat_f(struct sip_msg *msg, pv_param_t *param,
 	return rtpengine_rtpp_set_wrap(msg, rtpengine_rtpstat_wrap, parms, 1);
 }
 
-int set_rtp_inst_pvar(struct sip_msg *msg, const str * const uri) {
+static int
+set_rtp_inst_pvar(struct sip_msg *msg, const str * const uri) {
 	pv_value_t val;
 
 	if (rtp_inst_pvar == NULL)

modules/rtpengine/rtpengine.h

@@ -61,7 +61,6 @@ struct rtpp_set_head{
 struct rtpp_set *get_rtpp_set(int set_id);
 int add_rtpengine_socks(struct rtpp_set * rtpp_list, char * rtpproxy);
 
-int set_rtp_inst_pvar(struct sip_msg *msg, const str * const uri);
 
 int init_rtpproxy_db(void);
 

modules/rtpengine/rtpengine_hash.c

@@ -22,13 +22,12 @@ static int str_cmp_str(const str *a, const str *b) {
 }
 
 /* from sipwise rtpengine */
-static int str_equal(void *a, void *b) {
-	return (str_cmp_str((str *) a, (str *) b) == 0);
+static int str_equal(str *a, str *b) {
+	return (str_cmp_str(a, b) == 0);
 }
 
 /* from sipwise rtpengine */
-static unsigned int str_hash(void *ss) {
-	const str *s = (str*) ss;
+static unsigned int str_hash(str *s) {
 	unsigned int ret = 5381;
 	str it = *s;
 
@@ -62,14 +61,22 @@ int rtpengine_hash_table_init(int size) {
 
 	// init hashtable entry_list
 	rtpengine_hash_table->entry_list = shm_malloc(hash_table_size * sizeof(struct rtpengine_hash_entry));
+	if (!rtpengine_hash_table->entry_list) {
+		LM_ERR("no shm left to create rtpengine_hash_table->entry_list\n");
+		rtpengine_hash_table_destroy();
+		return 0;
+	}
+	memset(rtpengine_hash_table->entry_list, 0, hash_table_size * sizeof(struct rtpengine_hash_entry));
 
-	// init hashtable entry_list[i] (head never filled)
+	// init hashtable entry_list[i] (head never filled); destroy table on error
 	for (i = 0; i < hash_table_size; i++) {
 		rtpengine_hash_table->entry_list[i] = shm_malloc(sizeof(struct rtpengine_hash_entry));
 		if (!rtpengine_hash_table->entry_list[i]) {
 			LM_ERR("no shm left to create rtpengine_hash_table->entry_list[%d]\n", i);
+			rtpengine_hash_table_destroy();
 			return 0;
 		}
+		memset(rtpengine_hash_table->entry_list[i], 0, sizeof(struct rtpengine_hash_entry));
 
 		// never expire the head of the hashtable index lists
 		rtpengine_hash_table->entry_list[i]->tout = -1;
@@ -81,6 +88,7 @@ int rtpengine_hash_table_init(int size) {
 	rtpengine_hash_lock = lock_alloc();
 	if (!rtpengine_hash_lock) {
 		LM_ERR("no shm left to init rtpengine_hash_table lock");
+		rtpengine_hash_table_destroy();
 		return 0;
 	}
 
@@ -97,6 +105,14 @@ int rtpengine_hash_table_destroy() {
 		return 0;
 	}
 
+	// check rtpengine hashtable->entry_list
+	if (!rtpengine_hash_table->entry_list) {
+		LM_ERR("NULL rtpengine_hash_table->entry_list");
+		shm_free(rtpengine_hash_table);
+		rtpengine_hash_table = NULL;
+		return 0;
+	}
+
 	// destroy hashtable entry_list[i]
 	lock_get(rtpengine_hash_lock);
 	for (i = 0; i < hash_table_size; i++) {
@@ -111,6 +127,7 @@ int rtpengine_hash_table_destroy() {
 
 	// destroy hashtable entry_list
 	shm_free(rtpengine_hash_table->entry_list);
+	rtpengine_hash_table->entry_list = NULL;
 
 	// destroy hashtable
 	shm_free(rtpengine_hash_table);
@@ -128,7 +145,7 @@ int rtpengine_hash_table_destroy() {
 	return 1;
 }
 
-int rtpengine_hash_table_insert(void *key, void *value) {
+int rtpengine_hash_table_insert(str *key, struct rtpengine_hash_entry *value) {
 	struct rtpengine_hash_entry *entry, *last_entry;
 	struct rtpengine_hash_entry *new_entry = (struct rtpengine_hash_entry *) value;
 	unsigned int hash_index;
@@ -139,6 +156,12 @@ int rtpengine_hash_table_insert(void *key, void *value) {
 		return 0;
 	}
 
+	// check rtpengine hashtable->entry_list
+	if (!rtpengine_hash_table->entry_list) {
+		LM_ERR("NULL rtpengine_hash_table->entry_list");
+		return 0;
+	}
+
 	// get entry list
 	hash_index = str_hash(key);
 	entry = rtpengine_hash_table->entry_list[hash_index];
@@ -187,7 +210,7 @@ int rtpengine_hash_table_insert(void *key, void *value) {
 	return 1;
 }
 
-int rtpengine_hash_table_remove(void *key) {
+int rtpengine_hash_table_remove(str *key) {
 	struct rtpengine_hash_entry *entry, *last_entry;
 	unsigned int hash_index;
 
@@ -197,6 +220,12 @@ int rtpengine_hash_table_remove(void *key) {
 		return 0;
 	}
 
+	// check rtpengine hashtable->entry_list
+	if (!rtpengine_hash_table->entry_list) {
+		LM_ERR("NULL rtpengine_hash_table->entry_list");
+		return 0;
+	}
+
 	// get first entry from entry list; jump over unused list head
 	hash_index = str_hash(key);
 	entry = rtpengine_hash_table->entry_list[hash_index];
@@ -247,14 +276,21 @@ int rtpengine_hash_table_remove(void *key) {
 	return 0;
 }
 
-void* rtpengine_hash_table_lookup(void *key) {
+struct rtpp_node *rtpengine_hash_table_lookup(str *key) {
 	struct rtpengine_hash_entry *entry, *last_entry;
 	unsigned int hash_index;
+	struct rtpp_node *node;
 
 	// check rtpengine hashtable
 	if (!rtpengine_hash_table) {
 		LM_ERR("NULL rtpengine_hash_table");
-		return 0;
+		return NULL;
+	}
+
+	// check rtpengine hashtable->entry_list
+	if (!rtpengine_hash_table->entry_list) {
+		LM_ERR("NULL rtpengine_hash_table->entry_list");
+		return NULL;
 	}
 
 	// get first entry from entry list; jump over unused list head
@@ -267,10 +303,12 @@ void* rtpengine_hash_table_lookup(void *key) {
 	while (entry) {
 		// if key found, return entry
 		if (str_equal(&entry->callid, (str *)key)) {
+			node = entry->node;
+
 			// unlock
 			lock_release(rtpengine_hash_lock);
 
-			return entry;
+			return node;
 		}
 
 		// if expired entry discovered, delete it
@@ -310,6 +348,12 @@ void rtpengine_hash_table_print() {
 		return ;
 	}
 
+	// check rtpengine hashtable->entry_list
+	if (!rtpengine_hash_table->entry_list) {
+		LM_ERR("NULL rtpengine_hash_table->entry_list");
+		return ;
+	}
+
 	// lock
 	lock_get(rtpengine_hash_lock);
 

modules/rtpengine/rtpengine_hash.h

@@ -22,9 +22,9 @@ struct rtpengine_hash_table {
 
 int rtpengine_hash_table_init(int size);
 int rtpengine_hash_table_destroy();
-int rtpengine_hash_table_insert(void *key, void *value);
-int rtpengine_hash_table_remove(void *key);
-void* rtpengine_hash_table_lookup(void *key);
+int rtpengine_hash_table_insert(str *key, struct rtpengine_hash_entry *value);
+int rtpengine_hash_table_remove(str *key);
+struct rtpp_node *rtpengine_hash_table_lookup(str *key);
 void rtpengine_hash_table_print();
 unsigned int rtpengine_hash_table_total();