Skip to content

Commit

Permalink
keepalive: added uuid to ka_dest structure to avoid passing the whole…
Browse files Browse the repository at this point in the history
… struct to tm

- This avoids a race condition that may happen processing tm_request callbacl
- Allows to identify uniquely a ka_dest record
  • Loading branch information
NGSegovia committed Sep 1, 2020
1 parent 17d6a88 commit aaded16
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 3 deletions.
5 changes: 5 additions & 0 deletions src/modules/keepalive/keepalive.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
#include <time.h>
#include "../../core/sr_module.h"
#include "../../core/locking.h"
#include "../../core/str.h"
#include "../../core/utils/sruid.h"
#include "../tm/tm_load.h"

#define KA_INACTIVE_DST 1 /*!< inactive destination */
Expand Down Expand Up @@ -60,6 +62,7 @@ typedef struct _ka_dest
str uri;
str owner; // name of destination "owner"
// (module asking to monitor this destination
str uuid; // Universal id for this record
int flags;
int state;
time_t last_checked, last_up, last_down;
Expand All @@ -85,6 +88,7 @@ typedef struct _ka_destinations_list

extern ka_destinations_list_t *ka_destinations_list;
extern int ka_counter_del;
extern sruid_t ka_sruid;

ticks_t ka_check_timer(ticks_t ticks, struct timer_ln* tl, void* param);

Expand All @@ -96,6 +100,7 @@ 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_find_destination_by_uuid(str uuid, ka_dest_t **target, ka_dest_t **head);
int ka_lock_destination_list();
int ka_unlock_destination_list();
#endif
39 changes: 39 additions & 0 deletions src/modules/keepalive/keepalive_api.c
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,10 @@ int ka_add_dest(str *uri, str *owner, int flags, int ping_interval,
if(ka_str_copy(owner, &(dest->owner), NULL) < 0)
goto err;

if(sruid_next(&ka_sruid) < 0)
goto err;
ka_str_copy(&(dest->uuid), &(ka_sruid.uid), NULL);

dest->flags = flags;
dest->statechanged_clb = statechanged_clb;
dest->response_clb = response_clb;
Expand Down Expand Up @@ -236,6 +240,38 @@ int ka_find_destination(str *uri, str *owner, ka_dest_t **target, ka_dest_t **he
return 0;

}

/*!
* @function ka_find_destination_by_uuid
*
* @param *uuid uuid of ka_dest record
* @param **target searched address in stack
* @param **head which points target
* *
* @result 1 successful , 0 fail
*/
int ka_find_destination_by_uuid(str uuid, ka_dest_t **target, ka_dest_t **head){
ka_dest_t *dest=0 ,*temp=0;

LM_DBG("finding destination with uuid:%.*s\n", uuid.len, uuid.s);

for(dest = ka_destinations_list->first ;dest ; temp = dest, dest = dest->next ){
if(!dest)
break;

if (STR_EQ(uuid, dest->uuid) == 0){
*head = temp;
*target = dest;
LM_DBG("destination is found [target : %p] [head : %p] \r\n",target,temp);
return 1;
}
}

return 0;

}


/*!
* @function free_destination
* @abstract free ka_dest_t members
Expand All @@ -259,6 +295,9 @@ int free_destination(ka_dest_t *dest){
if(dest->owner.s)
shm_free(dest->owner.s);

if(dest->uuid.s)
shm_free(dest->uuid.s);

shm_free(dest);
}

Expand Down
15 changes: 12 additions & 3 deletions src/modules/keepalive/keepalive_core.c
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,14 @@ ticks_t ka_check_timer(ticks_t ticks, struct timer_ln* tl, void* param)
return (ticks_t)(0); /* stops the timer */
}

str *uuid = shm_malloc(sizeof(str));

This comment has been minimized.

Copy link
@sagarmalam

sagarmalam Sep 9, 2020

@NGSegovia , Memory allocated here is not freed at the end of function ? Could this be causing this issue ?
kamailio#2448 (comment)

This comment has been minimized.

Copy link
@NGSegovia

NGSegovia Sep 11, 2020

Author Owner

Not really, that's being freed in options_callback. But I was not freeing the content of the str only the struct. Can you give a try to this branch kamailio#2474 ?

This comment has been minimized.

Copy link
@sagarmalam

sagarmalam Sep 11, 2020

It is working well so far. I will let it run for 24 hours and get back. Thanks

This comment has been minimized.

Copy link
@sagarmalam

sagarmalam Sep 12, 2020

@NGSegovia , I encountered another problem where kamailio stops processing any traffic after it sends first batch of KAs.
I have ensured i am using correct branch this time : [root@SBC-4-2 kamailio]# git branch

  • keepalive_solve_sync_problems
    master
    [root@SBC-4-2 kamailio]# git pull origin keepalive_solve_sync_problems
    From https://github.com/NGSegovia/kamailio
  • branch keepalive_solve_sync_problems -> FETCH_HEAD
    Already up-to-date.
    [root@SBC-4-2 kamailio]#

I dont see any errors in kamailio logs. I am not sure what kind of information i can provide to help you troubleshoot this issue so please get back to me in case you need any other information

This comment has been minimized.

Copy link
@NGSegovia

NGSegovia Sep 13, 2020

Author Owner

I've updated my branch. Can you please give another try? Thanks @sagarmalam !

This comment has been minimized.

Copy link
@NGSegovia

NGSegovia Sep 15, 2020

Author Owner

Any news @sagarmalam ?

This comment has been minimized.

Copy link
@sagarmalam

sagarmalam Sep 16, 2020

@NGSegovia , Yes it is working fine now. Thanks a lot.

P.S : Sorry for late reply I wanted to run it for least couple of days before confirming it.

ka_str_copy(uuid, &(ka_dest->uuid), NULL);
/* Send ping using TM-Module.
* int request(str* m, str* ruri, str* to, str* from, str* h,
* str* b, str *oburi,
* transaction_cb cb, void* cbp); */
set_uac_req(&uac_r, &ka_ping_method, 0, 0, 0, TMCB_LOCAL_COMPLETED,
ka_options_callback, (void *)ka_dest);
ka_options_callback, (void *)uuid);

if(tmb.t_request(&uac_r, &ka_dest->uri, &ka_dest->uri, &ka_ping_from,
&ka_outbound_proxy)
Expand All @@ -99,8 +101,15 @@ static void ka_options_callback(

char *state_routes[] = {"", "keepalive:dst-up", "keepalive:dst-down"};

//NOTE: how to be sure destination is still allocated ?
ka_dest_t *ka_dest = (ka_dest_t *)(*ps->param);
str *uuid = (str *)(*ps->param);

// Retrieve ka_dest by uuid from destination list
ka_dest_t *ka_dest=0,*hollow=0;
if (!ka_find_destination_by_uuid(*uuid, &ka_dest, &hollow)) {
LM_ERR("Couldn't find destination \r\n");
return;
}
shm_free(uuid);

uri.s = t->to.s + 5;
uri.len = t->to.len - 8;
Expand Down
5 changes: 5 additions & 0 deletions src/modules/keepalive/keepalive_mod.c
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ extern struct tm_binds tmb;

int ka_ping_interval = 30;
ka_destinations_list_t *ka_destinations_list = NULL;
sruid_t ka_sruid;
str ka_ping_from = str_init("sip:keepalive@kamailio.org");
int ka_counter_del = 5;

Expand Down Expand Up @@ -124,6 +125,10 @@ static int mod_init(void)
if(ka_alloc_destinations_list() < 0)
return -1;

if(sruid_init(&ka_sruid, '-', "ka", SRUID_INC) < 0) {
return -1;
}

return 0;
}

Expand Down

3 comments on commit aaded16

@davesoft11
Copy link

Choose a reason for hiding this comment

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

Should these new exception handlers also free memory before leaving so we don't have more memory leaks?

	if (!ka_find_destination_by_uuid(*uuid, &ka_dest, &hollow)) {
		LM_ERR("Couldn't find destination \r\n");
		return;
	}
	if(sruid_init(&ka_sruid, '-', "ka", SRUID_INC) < 0) {
		return -1;
	}

@NGSegovia
Copy link
Owner Author

Choose a reason for hiding this comment

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

Hi @davesoft11 ,
The first one is already addressed in https://github.com/kamailio/kamailio/pull/2474/files Pull request (these changes has been already merged).

About the second one there is no "sruid_destroy" function, and I checked in other modules just initialized and used the same way. Anyway that happens in the mod_init function so it's likely to be destroyed anyway in Kamailio's stop.

Thanks!

@davesoft11
Copy link

Choose a reason for hiding this comment

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

The first one is already addressed in https://github.com/kamailio/kamailio/pull/2474/files Pull request (these changes has been already merged).

About the second one there is no "sruid_destroy" function, and I checked in other modules just initialized and used the same way. Anyway that happens in the mod_init function so it's likely to be destroyed anyway in Kamailio's stop.

Okay, thanks for the reply.

Please sign in to comment.