[sr-dev] [kamailio/kamailio] keepalive : added new function del_destination and added cfg functions (#2133)

[Daniel-Constantin Mierla]

miconda requested changes on this pull request.



>  
-	LM_INFO("adding destination: %.*s\n", uri->len, uri->s);
+	LM_DBG("adding destination: %.*s\n", uri->len, uri->s);
+
+	if(ka_find_destination(uri , owner , &dest , &dest)){
+		LM_INFO("uri [%.*s] already in stack --ignoring \r\n",uri->len, uri->s);
+		dest->counter=0;

The change of the `dest->counter` is done out of the mutex zone and this operation can be done on an invalid structure if the `dest` was removed meanwhile.

> +* @abstract deletes given sip uri in allocated destination stack as named ka_alloc_destinations_list
+*
+* @param msg sip message
+* @param uri given uri
+* @param owner given owner name, not using now
+*	*
+* @result 1 successful  , -1 fail
+*/
+int ka_del_destination(str *uri, str *owner){
+
+	ka_dest_t *target=0,*head=0;
+
+	if(!ka_find_destination(uri,owner,&target,&head)){
+		LM_ERR("Couldnt find destination \r\n");
+		return -1;
+	}

Similar issues, `target` and `head` are retrieved from `ka_find_destination()`, followed by code out of the mutex zone  and later used directly, but at that time any of them can be already deleted. So removal has to be done in the same mutex zone as it is searched.

Actually I do not see any good use for  `ka_find_destination()` returning `target` and `head`. The function can be useful to know if the address exists, but using `target` and `head` after this function expose to segfault. Either you keep the lock set in the function at the return time and unlock later, out of the function, or just make it to return true/false on finding the destination, without returning `target` and `head`.

>  
 extern struct tm_binds tmb;
 
 int ka_ping_interval = 30;
 ka_destinations_list_t *ka_destinations_list = NULL;
+str ka_ping_from = str_init("sip:keepalive at kamailio.org");
+int counter_del = 5;

I suggest adding `ka_` prefix to global variables that are not static and exposed in other files via `extern`.

>  
 
 static cmd_export_t cmds[] = {
 	{"is_alive", (cmd_function)w_cmd_is_alive, 1,
 			fixup_spve_null, 0, ANY_ROUTE},
 	// internal API
+	{"add_destination", (cmd_function)w_add_destination, 2,
+		fixup_add_destination, 0, REQUEST_ROUTE|BRANCH_ROUTE|ONREPLY_ROUTE},
+	{"del_destination", (cmd_function)w_del_destination, 2,
+		fixup_add_destination, 0, ANY_ROUTE},

As more functions are exported from this module, I think is better to also prefix the exported name with `ka_`, like most modules use a common prefix for their functions. Probably we should also add an alias named  `ka_is_alive()` for `is_alive()`.

> @@ -126,6 +138,8 @@ static int mod_init(void)
  */
 static void mod_destroy(void)
 {
+	lock_release(ka_destinations_list->lock);
+	lock_dealloc(ka_destinations_list->lock);
 }

This two have to be enclosed in `if(ka_destinations_list) { ... }`, the mod_destroy() is executed even when mod_init() is not finished, in that case ka_destinations_list is NULL, crashing kamailio at shutdown.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2133#pullrequestreview-325064799
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.kamailio.org/pipermail/sr-dev/attachments/20191202/3918c278/attachment-0001.html>