rtpengine: Fix deletion for branching scenarios

- hash table entry contains callid, viabranch
- hash table lookup based on callid, viabranch (useful for branching scenarios);
keep doing the hash table remove right away
- remove op param when select_rtpp_node(); not needed

modules/rtpengine/rtpengine.c

@@ -193,9 +193,9 @@ static int rtpengine_offer_answer(struct sip_msg *msg, const char *flags, int op
 static int fixup_set_id(void ** param, int param_no);
 static int set_rtpengine_set_f(struct sip_msg * msg, char * str1, char * str2);
 static struct rtpp_set * select_rtpp_set(int id_set);
-static struct rtpp_node *select_rtpp_node_new(str, int, int);
-static struct rtpp_node *select_rtpp_node_old(str, int, int);
-static struct rtpp_node *select_rtpp_node(str, int, int);
+static struct rtpp_node *select_rtpp_node_new(str, str, int);
+static struct rtpp_node *select_rtpp_node_old(str, str, int);
+static struct rtpp_node *select_rtpp_node(str, str, int);
 static char *send_rtpp_command(struct rtpp_node *, bencode_item_t *, int *);
 static int get_extra_id(struct sip_msg* msg, str *id_str);
 
@@ -1859,7 +1859,8 @@ static bencode_item_t *rtpp_function_call(bencode_buffer_t *bencbuf, struct sip_
 {
 	struct ng_flags_parse ng_flags;
 	bencode_item_t *item, *resp;
-	str callid, from_tag, to_tag, body, viabranch, error;
+	str callid = STR_NULL, from_tag = STR_NULL, to_tag = STR_NULL, viabranch = STR_NULL;
+	str body = STR_NULL, error = STR_NULL;
 	int ret, queried_nodes;
 	struct rtpp_node *node;
 	char *cp;
@@ -1992,7 +1993,7 @@ select_node:
 			LM_ERR("queried nodes limit reached\n");
 			goto error;
 		}
-		node = select_rtpp_node(callid, 1, op);
+		node = select_rtpp_node(callid, viabranch, 1);
 		if (!node) {
 			LM_ERR("no available proxies\n");
 			goto error;
@@ -2036,12 +2037,12 @@ select_node:
 
 	if (op == OP_DELETE) {
 		/* Delete the key<->value from the hashtable */
-		if (!rtpengine_hash_table_remove(&callid)) {
-			LM_ERR("rtpengine hash table failed to remove entry for callen=%d callid=%.*s\n",
-				callid.len, callid.len, callid.s);
+		if (!rtpengine_hash_table_remove(callid, viabranch)) {
+			LM_ERR("rtpengine hash table failed to remove entry for callen=%d callid=%.*s viabranch=%.*s\n",
+				callid.len, callid.len, callid.s, viabranch.len, viabranch.s);
 		} else {
-			LM_DBG("rtpengine hash table remove entry for callen=%d callid=%.*s\n",
-				callid.len, callid.len, callid.s);
+			LM_DBG("rtpengine hash table remove entry for callen=%d callid=%.*s viabranch=%.*s\n",
+				callid.len, callid.len, callid.s, viabranch.len, viabranch.s);
 		}
 	}
 
@@ -2278,7 +2279,7 @@ static struct rtpp_set * select_rtpp_set(int id_set ){
  * run the selection algorithm and return the new selected node
  */
 static struct rtpp_node *
-select_rtpp_node_new(str callid, int do_test, int op)
+select_rtpp_node_new(str callid, str viabranch, int do_test)
 {
 	struct rtpp_node* node;
 	unsigned i, sum, sumcut, weight_sum;
@@ -2350,18 +2351,25 @@ found:
 	}
 
 	/* build the entry */
-	struct rtpengine_hash_entry *entry = shm_malloc(sizeof(struct rtpp_node));
+	struct rtpengine_hash_entry *entry = shm_malloc(sizeof(struct rtpengine_hash_entry));
 	if (!entry) {
-		LM_ERR("rtpengine hash table fail to create entry for calllen=%d callid=%.*s\n",
-			callid.len, callid.len, callid.s);
+		LM_ERR("rtpengine hash table fail to create entry for calllen=%d callid=%.*s viabranch=%.*s\n",
+			callid.len, callid.len, callid.s, viabranch.len, viabranch.s);
 		return node;
 	}
+        memset(entry, 0, sizeof(struct rtpengine_hash_entry));
 
 	/* fill the entry */
 	if (shm_str_dup(&entry->callid, &callid) < 0) {
 		LM_ERR("rtpengine hash table fail to duplicate calllen=%d callid=%.*s\n",
 			callid.len, callid.len, callid.s);
-		shm_free(entry);
+		rtpengine_hash_table_free_entry(entry);
+		return node;
+	}
+	if (shm_str_dup(&entry->viabranch, &viabranch) < 0) {
+		LM_ERR("rtpengine hash table fail to duplicate calllen=%d viabranch=%.*s\n",
+			callid.len, viabranch.len, viabranch.s);
+		rtpengine_hash_table_free_entry(entry);
 		return node;
 	}
 	entry->node = node;
@@ -2369,15 +2377,14 @@ found:
 	entry->tout = get_ticks() + hash_table_tout;
 
 	/* 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\n",
-			node->rn_url.len, node->rn_url.s, callid.len, callid.len, callid.s);
-		shm_free(entry->callid.s);
-		shm_free(entry);
+	if (!rtpengine_hash_table_insert(callid, viabranch, entry)) {
+		LM_ERR("rtpengine hash table fail to insert node=%.*s for calllen=%d callid=%.*s viabranch=%.*s\n",
+			node->rn_url.len, node->rn_url.s, callid.len, callid.len, callid.s, viabranch.len, viabranch.s);
+		rtpengine_hash_table_free_entry(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);
+		LM_DBG("rtpengine hash table insert node=%.*s for calllen=%d callid=%.*s viabranch=%.*s\n",
+			node->rn_url.len, node->rn_url.s, callid.len, callid.len, callid.s, viabranch.len, viabranch.s);
 	}
 
 	/* return selected node */
@@ -2388,19 +2395,19 @@ found:
  * lookup the hastable (key=callid value=node) and get the old node (e.g. for answer/delete)
  */
 static struct rtpp_node *
-select_rtpp_node_old(str callid, int do_test, int op)
+select_rtpp_node_old(str callid, str viabranch, int do_test)
 {
 	struct rtpp_node *node = NULL;
 
-	node = rtpengine_hash_table_lookup(&callid);
+	node = rtpengine_hash_table_lookup(callid, viabranch);
 
 	if (!node) {
-		LM_NOTICE("rtpengine hash table lookup failed to find node for calllen=%d callid=%.*s\n",
-			callid.len, callid.len, callid.s);
+		LM_NOTICE("rtpengine hash table lookup failed to find node for calllen=%d callid=%.*s viabranch=%.*s\n",
+			callid.len, callid.len, callid.s, viabranch.len, viabranch.s);
 		return NULL;
 	} else {
-		LM_DBG("rtpengine hash table lookup find node=%.*s for calllen=%d callid=%.*s\n",
-			node->rn_url.len, node->rn_url.s, callid.len, callid.len, callid.s);
+		LM_DBG("rtpengine hash table lookup find node=%.*s for calllen=%d callid=%.*s viabranch=%.*s\n",
+			node->rn_url.len, node->rn_url.s, callid.len, callid.len, callid.s, viabranch.len, viabranch.s);
 	}
 
 	return node;
@@ -2411,7 +2418,7 @@ select_rtpp_node_old(str callid, int do_test, int op)
  * 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)
+select_rtpp_node(str callid, str viabranch, int do_test)
 {
 	struct rtpp_node *node = NULL;
 
@@ -2421,12 +2428,12 @@ select_rtpp_node(str callid, int do_test, int op)
 	}
 
 	// lookup node
-	node = select_rtpp_node_old(callid, do_test, op);
+	node = select_rtpp_node_old(callid, viabranch, do_test);
 
 	// check node
 	if (!node) {
 		// run the selection algorithm
-		node = select_rtpp_node_new(callid, do_test, op);
+		node = select_rtpp_node_new(callid, viabranch, do_test);
 
 		// check node
 		if (!node) {

modules/rtpengine/rtpengine_hash.c

@@ -11,25 +11,25 @@ static struct rtpengine_hash_table *rtpengine_hash_table;
 static int hash_table_size;
 
 /* from sipwise rtpengine */
-static int str_cmp_str(const str *a, const str *b) {
-	if (a->len < b->len)
+static int str_cmp_str(const str a, const str b) {
+	if (a.len < b.len)
 		return -1;
-	if (a->len > b->len)
+	if (a.len > b.len)
 		return 1;
-	if (a->len == 0 && b->len == 0)
+	if (a.len == 0 && b.len == 0)
 		return 0;
-	return memcmp(a->s, b->s, a->len);
+	return memcmp(a.s, b.s, a.len);
 }
 
 /* from sipwise rtpengine */
-static int str_equal(str *a, str *b) {
+static int str_equal(str a, str b) {
 	return (str_cmp_str(a, b) == 0);
 }
 
 /* from sipwise rtpengine */
-static unsigned int str_hash(str *s) {
+static unsigned int str_hash(str s) {
 	unsigned int ret = 5381;
-	str it = *s;
+	str it = s;
 
 	while (it.len > 0) {
 		ret = (ret << 5) + ret + *it.s;
@@ -40,7 +40,7 @@ static unsigned int str_hash(str *s) {
 	return ret % hash_table_size;
 }
 
-/* rtpengine glib hash API */
+/* rtpengine hash API */
 int rtpengine_hash_table_init(int size) {
 	int i;
 
@@ -58,6 +58,7 @@ int rtpengine_hash_table_init(int size) {
 		LM_ERR("no shm left to create rtpengine_hash_table\n");
 		return 0;
 	}
+	memset(rtpengine_hash_table, 0, sizeof(struct rtpengine_hash_table));
 
 	// init hashtable entry_list
 	rtpengine_hash_table->entry_list = shm_malloc(hash_table_size * sizeof(struct rtpengine_hash_entry));
@@ -97,7 +98,6 @@ int rtpengine_hash_table_init(int size) {
 
 int rtpengine_hash_table_destroy() {
 	int i;
-	struct rtpengine_hash_entry *entry, *last_entry;
 
 	// check rtpengine hashtable
 	if (!rtpengine_hash_table) {
@@ -113,16 +113,11 @@ int rtpengine_hash_table_destroy() {
 		return 0;
 	}
 
-	// destroy hashtable entry_list[i]
 	lock_get(rtpengine_hash_lock);
+
+	// destroy hashtable entry_list[i]
 	for (i = 0; i < hash_table_size; i++) {
-		entry = rtpengine_hash_table->entry_list[i];
-		while (entry) {
-			last_entry = entry;
-			entry = entry->next;
-			shm_free(last_entry->callid.s);
-			shm_free(last_entry);
-		}
+		rtpengine_hash_table_free_entry_list(rtpengine_hash_table->entry_list[i]);
 	}
 
 	// destroy hashtable entry_list
@@ -132,6 +127,7 @@ int rtpengine_hash_table_destroy() {
 	// destroy hashtable
 	shm_free(rtpengine_hash_table);
 	rtpengine_hash_table = NULL;
+
 	lock_release(rtpengine_hash_lock);
 
 	// destroy lock
@@ -145,7 +141,7 @@ int rtpengine_hash_table_destroy() {
 	return 1;
 }
 
-int rtpengine_hash_table_insert(str *key, struct rtpengine_hash_entry *value) {
+int rtpengine_hash_table_insert(str callid, str viabranch, 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;
@@ -163,18 +159,21 @@ int rtpengine_hash_table_insert(str *key, struct rtpengine_hash_entry *value) {
 	}
 
 	// get entry list
-	hash_index = str_hash(key);
+	hash_index = str_hash(callid);
 	entry = rtpengine_hash_table->entry_list[hash_index];
 	last_entry = entry;
 
 	// lock
 	lock_get(rtpengine_hash_lock);
 	while (entry) {
-		// if key found, don't add new entry
-		if (str_equal(&entry->callid, &new_entry->callid)) {
+		// if found, don't add new entry
+		if (str_equal(entry->callid, new_entry->callid) &&
+		    str_equal(entry->viabranch, new_entry->viabranch)) {
 			// unlock
 			lock_release(rtpengine_hash_lock);
-			LM_ERR("Call id %.*s already in hashtable, ignore new value", entry->callid.len, entry->callid.s);
+			LM_NOTICE("callid=%.*s, viabranch=%.*s already in hashtable, ignore new value",
+				entry->callid.len, entry->callid.s,
+				entry->viabranch.len, entry->viabranch.s);
 			return 0;
 		}
 
@@ -184,8 +183,7 @@ int rtpengine_hash_table_insert(str *key, struct rtpengine_hash_entry *value) {
 			last_entry->next = entry->next;
 
 			// free current entry; entry points to unknown
-			shm_free(entry->callid.s);
-			shm_free(entry);
+			rtpengine_hash_table_free_entry(entry);
 
 			// set pointers
 			entry = last_entry;
@@ -210,7 +208,7 @@ int rtpengine_hash_table_insert(str *key, struct rtpengine_hash_entry *value) {
 	return 1;
 }
 
-int rtpengine_hash_table_remove(str *key) {
+int rtpengine_hash_table_remove(str callid, str viabranch) {
 	struct rtpengine_hash_entry *entry, *last_entry;
 	unsigned int hash_index;
 
@@ -227,19 +225,19 @@ int rtpengine_hash_table_remove(str *key) {
 	}
 
 	// get first entry from entry list; jump over unused list head
-	hash_index = str_hash(key);
+	hash_index = str_hash(callid);
 	entry = rtpengine_hash_table->entry_list[hash_index];
 	last_entry = entry;
 
 	// lock
 	lock_get(rtpengine_hash_lock);
 	while (entry) {
-		// if key found, delete entry
-		if (str_equal(&entry->callid, (str *)key)) {
+		// if callid found, delete entry
+		if (str_equal(entry->callid, callid) &&
+		    str_equal(entry->viabranch, viabranch)) {
 			// free entry
 			last_entry->next = entry->next;
-			shm_free(entry->callid.s);
-			shm_free(entry);
+			rtpengine_hash_table_free_entry(entry);
 
 			// update total
 			rtpengine_hash_table->total--;
@@ -256,8 +254,7 @@ int rtpengine_hash_table_remove(str *key) {
 			last_entry->next = entry->next;
 
 			// free current entry; entry points to unknown
-			shm_free(entry->callid.s);
-			shm_free(entry);
+			rtpengine_hash_table_free_entry(entry);
 
 			// set pointers
 			entry = last_entry;
@@ -276,7 +273,7 @@ int rtpengine_hash_table_remove(str *key) {
 	return 0;
 }
 
-struct rtpp_node *rtpengine_hash_table_lookup(str *key) {
+struct rtpp_node *rtpengine_hash_table_lookup(str callid, str viabranch) {
 	struct rtpengine_hash_entry *entry, *last_entry;
 	unsigned int hash_index;
 	struct rtpp_node *node;
@@ -294,15 +291,16 @@ struct rtpp_node *rtpengine_hash_table_lookup(str *key) {
 	}
 
 	// get first entry from entry list; jump over unused list head
-	hash_index = str_hash(key);
+	hash_index = str_hash(callid);
 	entry = rtpengine_hash_table->entry_list[hash_index];
 	last_entry = entry;
 
 	// lock
 	lock_get(rtpengine_hash_lock);
 	while (entry) {
-		// if key found, return entry
-		if (str_equal(&entry->callid, (str *)key)) {
+		// if callid found, return entry
+		if (str_equal(entry->callid, callid) &&
+		    str_equal(entry->viabranch, viabranch)) {
 			node = entry->node;
 
 			// unlock
@@ -317,8 +315,7 @@ struct rtpp_node *rtpengine_hash_table_lookup(str *key) {
 			last_entry->next = entry->next;
 
 			// free current entry; entry points to unknown
-			shm_free(entry->callid.s);
-			shm_free(entry);
+			rtpengine_hash_table_free_entry(entry);
 
 			// set pointers
 			entry = last_entry;
@@ -369,8 +366,7 @@ void rtpengine_hash_table_print() {
 				last_entry->next = entry->next;
 
 				// free current entry; entry points to unknown
-				shm_free(entry->callid.s);
-				shm_free(entry);
+				rtpengine_hash_table_free_entry(entry);
 
 				// set pointers
 				entry = last_entry;
@@ -401,3 +397,41 @@ unsigned int rtpengine_hash_table_total() {
 
 	return rtpengine_hash_table->total;
 }
+
+void rtpengine_hash_table_free_entry(struct rtpengine_hash_entry *entry) {
+	if (!entry) {
+		return ;
+	}
+
+	// free callid
+	if (entry->callid.s) {
+		shm_free(entry->callid.s);
+	}
+
+	// free viabranch
+	if (entry->viabranch.s) {
+		shm_free(entry->viabranch.s);
+	}
+
+	// free entry
+	shm_free(entry);
+
+	return ;
+}
+
+void rtpengine_hash_table_free_entry_list(struct rtpengine_hash_entry *entry_list) {
+	struct rtpengine_hash_entry *entry, *last_entry;
+
+	if (!entry_list) {
+		return ;
+	}
+
+	entry = entry_list;
+	while (entry) {
+		last_entry = entry;
+		entry = entry->next;
+		rtpengine_hash_table_free_entry(last_entry);
+	}
+
+	return ;
+}

modules/rtpengine/rtpengine_hash.h

@@ -6,10 +6,11 @@
 
 /* table entry */
 struct rtpengine_hash_entry {
-	unsigned int tout;			// call timeout
 	str callid;				// call callid
+	str viabranch;				// call viabranch
 	struct rtpp_node *node;			// call selected node
 
+	unsigned int tout;			// call timeout
 	struct rtpengine_hash_entry *next;	// call next
 };
 
@@ -22,10 +23,13 @@ struct rtpengine_hash_table {
 
 int rtpengine_hash_table_init(int size);
 int rtpengine_hash_table_destroy();
-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);
+int rtpengine_hash_table_insert(str callid, str viabranch, struct rtpengine_hash_entry *value);
+int rtpengine_hash_table_remove(str callid, str viabranch);
+struct rtpp_node *rtpengine_hash_table_lookup(str callid, str viabranch);
 void rtpengine_hash_table_print();
 unsigned int rtpengine_hash_table_total();
 
+void rtpengine_hash_table_free_entry(struct rtpengine_hash_entry *entry);
+void rtpengine_hash_table_free_entry_list(struct rtpengine_hash_entry *entry_list);
+
 #endif