Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

keepalive : added new function del_destination and added cfg functions #2133

Merged
merged 3 commits into from Dec 12, 2019
Merged

keepalive : added new function del_destination and added cfg functions #2133

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
67abf1a
keepalive : added new function del_destination and added .cfg functions
ycaner06 Nov 16, 2019
54ae3c3
keepalive : fixed function names and re-placed un/lock functions
ycaner06 Dec 7, 2019
9289bfc
keepalive : added doc for exported new functions ka_add_destination a…
Dec 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/modules/keepalive/api.h
View file
Open in desktop
Expand Up @@ -42,13 +42,17 @@ typedef int (*ka_add_dest_f)(str *uri, str *owner, int flags,
typedef ka_state (*ka_dest_state_f)(str *uri);
typedef int (*ka_del_destination_f)(str *uri, str *owner);
typedef int (*ka_find_destination_f)(str *uri, str *owner,ka_dest_t **target,ka_dest_t **head);
typedef int (*ka_lock_destination_list_f)();
typedef int (*ka_unlock_destination_list_f)();

typedef struct keepalive_api
{
ka_add_dest_f add_destination;
ka_dest_state_f destination_state;
ka_del_destination_f del_destination;
ka_find_destination_f find_destination;
ka_lock_destination_list_f lock_destination_list;
ka_unlock_destination_list_f unlock_destination_list;
} keepalive_api_t;

typedef int (*bind_keepalive_f)(keepalive_api_t *api);
Expand Down
5 changes: 3 additions & 2 deletions src/modules/keepalive/keepalive.h
View file
Open in desktop
Expand Up @@ -74,7 +74,7 @@ typedef struct _ka_destinations_list
} ka_destinations_list_t;

extern ka_destinations_list_t *ka_destinations_list;
extern int counter_del;
extern int ka_counter_del;

int ka_add_dest(str *uri, str *owner, int flags, ka_statechanged_f callback,
void *user_attr);
Expand All @@ -83,5 +83,6 @@ int ka_str_copy(str *src, str *dest, char *prefix);
int free_destination(ka_dest_t *dest) ;
int ka_del_destination(str *uri, str *owner) ;
int ka_find_destination(str *uri, str *owner, ka_dest_t **target ,ka_dest_t **head);

int ka_lock_destination_list();
int ka_unlock_destination_list();
#endif
64 changes: 43 additions & 21 deletions src/modules/keepalive/keepalive_api.c
View file
Open in desktop
Expand Up @@ -54,6 +54,8 @@ int bind_keepalive(keepalive_api_t *api)
api->add_destination = ka_add_dest;
api->destination_state = ka_destination_state;
api->del_destination = ka_del_destination;
api->lock_destination_list = ka_lock_destination_list;
api->unlock_destination_list = ka_unlock_destination_list;
return 0;
}

Expand All @@ -64,13 +66,14 @@ int ka_add_dest(str *uri, str *owner, int flags, ka_statechanged_f callback,
void *user_attr)
{
struct sip_uri _uri;
ka_dest_t *dest=0;
ka_dest_t *dest=0,*hollow=0;

LM_DBG("adding destination: %.*s\n", uri->len, uri->s);

if(ka_find_destination(uri , owner , &dest , &dest)){
ka_lock_destination_list();
if(ka_find_destination(uri , owner , &dest , &hollow)){
LM_INFO("uri [%.*s] already in stack --ignoring \r\n",uri->len, uri->s);
dest->counter=0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ka_unlock_destination_list();
return -2;
}

Expand Down Expand Up @@ -107,7 +110,9 @@ int ka_add_dest(str *uri, str *owner, int flags, ka_statechanged_f callback,
dest->next = ka_destinations_list->first;
ka_destinations_list->first = dest;

return 0;
ka_unlock_destination_list();

return 1;

err:
if(dest) {
Expand All @@ -116,6 +121,8 @@ int ka_add_dest(str *uri, str *owner, int flags, ka_statechanged_f callback,

shm_free(dest);
}
ka_unlock_destination_list();

return -1;
}
/*
Expand All @@ -124,7 +131,7 @@ int ka_add_dest(str *uri, str *owner, int flags, ka_statechanged_f callback,
ka_state ka_destination_state(str *destination)
{
ka_dest_t *ka_dest = NULL;

ka_lock_destination_list();
for(ka_dest = ka_destinations_list->first; ka_dest != NULL;
ka_dest = ka_dest->next) {
if((destination->len == ka_dest->uri.len - 4)
Expand All @@ -133,7 +140,7 @@ ka_state ka_destination_state(str *destination)
break;
}
}

ka_unlock_destination_list();
if(ka_dest == NULL) {
return (-1);
}
Expand All @@ -153,36 +160,37 @@ ka_state ka_destination_state(str *destination)
int ka_del_destination(str *uri, str *owner){

ka_dest_t *target=0,*head=0;
ka_lock_destination_list();

if(!ka_find_destination(uri,owner,&target,&head)){
LM_ERR("Couldnt find destination \r\n");
return -1;
LM_ERR("Couldn't find destination \r\n");
goto err;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if(!target){
LM_ERR("Couldnt find destination \r\n");
return -1;
LM_ERR("Couldn't find destination \r\n");
goto err;
}

lock_get(ka_destinations_list->lock);

if(!head){
LM_DBG("There isnt any head so maybe it is first \r\n");
LM_DBG("There isn't any head so maybe it is first \r\n");
ka_destinations_list->first = target->next;
free_destination(target);
lock_release(ka_destinations_list->lock);
ka_unlock_destination_list();
return 1;
}
head->next = target->next;
free_destination(target);
lock_release(ka_destinations_list->lock);

ka_unlock_destination_list();
return 1;
err:
ka_unlock_destination_list();
return -1;
}
/*!
* @function ka_find_destination
* @abstract find given destination uri address in destination_list stack
*
* don't forget to add lock via ka_lock_destination_list to prevent crashes
* @param *uri given uri
* @param *owner given owner name, not using now
* @param **target searched address in stack
Expand All @@ -193,8 +201,8 @@ int ka_del_destination(str *uri, str *owner){
int ka_find_destination(str *uri, str *owner, ka_dest_t **target, ka_dest_t **head){

ka_dest_t *dest=0 ,*temp=0;
LM_DBG("finding destination: %.*s\n", uri->len, uri->s);

lock_get(ka_destinations_list->lock);
for(dest = ka_destinations_list->first ;dest; temp=dest, dest= dest->next ){
if(!dest)
break;
Expand All @@ -203,14 +211,12 @@ int ka_find_destination(str *uri, str *owner, ka_dest_t **target, ka_dest_t **he
continue;

if(memcmp(dest->uri.s , uri->s , uri->len>dest->uri.len?dest->uri.len : uri->len)==0){
*target = dest;
*head = temp;
*target = dest;
LM_DBG("destination is found [target : %p] [head : %p] \r\n",target,temp);
lock_release(ka_destinations_list->lock);
return 1;
}
}
lock_release(ka_destinations_list->lock);

return 0;

Expand All @@ -237,3 +243,19 @@ int free_destination(ka_dest_t *dest){

return 1;
}

int ka_lock_destination_list(){
if(ka_destinations_list){
lock_get(ka_destinations_list->lock);
return 1;
}
return 0;
}

int ka_unlock_destination_list(){
if(ka_destinations_list){
lock_release(ka_destinations_list->lock);
return 1;
}
return 0;
}
7 changes: 3 additions & 4 deletions src/modules/keepalive/keepalive_core.c
View file
Open in desktop
Expand Up @@ -61,7 +61,7 @@ void ka_check_timer(unsigned int ticks, void *param)

LM_DBG("ka check timer\n");

lock_get(ka_destinations_list->lock);
ka_lock_destination_list();

for(ka_dest = ka_destinations_list->first; ka_dest != NULL;
ka_dest = ka_dest->next) {
Expand All @@ -72,7 +72,7 @@ void ka_check_timer(unsigned int ticks, void *param)
* str* b, str *oburi,
* transaction_cb cb, void* cbp); */

if(ka_dest->counter>counter_del){
if(ka_dest->counter>ka_counter_del){
continue;
}

Expand All @@ -87,8 +87,7 @@ void ka_check_timer(unsigned int ticks, void *param)

ka_dest->last_checked = time(NULL);
}
lock_release(ka_destinations_list->lock);

ka_unlock_destination_list();

return;
}
Expand Down
16 changes: 9 additions & 7 deletions src/modules/keepalive/keepalive_mod.c
View file
Open in desktop
Expand Up @@ -63,17 +63,17 @@ 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@kamailio.org");
int counter_del = 5;
int ka_counter_del = 5;



static cmd_export_t cmds[] = {
{"is_alive", (cmd_function)w_cmd_is_alive, 1,
{"ka_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,
{"ka_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,
{"ka_del_destination", (cmd_function)w_del_destination, 2,
fixup_add_destination, 0, ANY_ROUTE},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

{"bind_keepalive", (cmd_function)bind_keepalive, 0, 0, 0, 0},
{0, 0, 0, 0, 0, 0}
Expand All @@ -85,7 +85,7 @@ static param_export_t params[] = {
{"destination", PARAM_STRING | USE_FUNC_PARAM,
(void *)ka_mod_add_destination},
{"ping_from", PARAM_STRING, &ka_ping_from},
{"delete_counter", PARAM_INT, &counter_del},
{"delete_counter", PARAM_INT, &ka_counter_del},
{0, 0, 0}
};

Expand Down Expand Up @@ -138,8 +138,10 @@ static int mod_init(void)
*/
static void mod_destroy(void)
{
lock_release(ka_destinations_list->lock);
lock_dealloc(ka_destinations_list->lock);
if(ka_destinations_list){
lock_release(ka_destinations_list->lock);
lock_dealloc(ka_destinations_list->lock);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



Expand Down