siptrace: use xavps to pass data for the duration of transaction/dialog #1963
Conversation
Before this data was serialized in order to fit a normal AVP and be passed to DLGCB_CREATED callback. Moreover for transaction tracing data was allocated in current process memory which would have crashed if the reply were to be recieved in a different process. With the current implementation data is allocated in shared memory, all processes having access to it. For dialogs data is passed through xavp to dlgcb created. From there all dialog callbacks are registered and they receive argument the pointer to siptrace info. For transactions the pointer is passed as dialog callback parameter.
|
The type SR_XTYPE_VPTR is not resulting in freeing the pointer when XAVP is destroyed, it was added to keep references to permanent structures kept in memory during runtime. So I just added (commit 126cfa5) the new type SR_XTYPE_SPTR which does a shm_free() on the pointer when the XAVP is destroyed. If trace info struct is stored permanently in shared memory during Kamailio runtime, then SR_XTYPE_VPTR is ok, otherwise the patch needs to be updated to set SR_XTYPE_SPTR. One more remark -- for tracking the dialog, if the trace info struct is not permanent, but destroyed with the XAVP, be sure you don't pass that pointer to dlg callbacks, because the life of XAVP is as long as transaction exist, but the dialog is taking longer. |
|
I saw SR_XTYPE_DATA(if i'm not wrong) which gives you the possibility to have a free function but didn't use it because of the following:
I tested the code against leaks for the following scenarions: successfull call, kamailio generated dual bye, INVITE failed with 491 and CANCEL scenario. There are no leaks. If you have other scenarios in mind I'll be glad to try them out. |
|
OK, thanks for clarification, I wanted to be sure about the relations between the xavp and dialog. Maybe @henningw or someone else wants to review as well. If no other objections, it will be merged very soon. |
|
|
||
| /* could use the dest_info we've already parsed but there's no way to pass | ||
| * it to DLGCB_CREATED callback so the only thing to do is keep | ||
| * it as uri, serialize in a dlg_var and parse again in DLGCB_CREATED */ | ||
| if(corid) { | ||
| info->correlation_id = *corid; | ||
| info->correlation_id.s = (char *)(info + 1); |
There was a problem hiding this comment.
Maybe I misunderstood the code here, but why you use +1? If you want to increment the pointer for siptrace_info_t length, maybe use sizeof?
There was a problem hiding this comment.
Arithmetics with pointers is ok, adding 1 is practically adding the size of respective structure to the initial address. In old times, arithmetic operation with pointers was known to be safe when the pointer was referring to an array element, therefore I preferred to do cast to char* for initial pointer and then add sizeof(struct), like: (char*)info + sizeof(siptrace_info_t). With modern compilers gcc/clang, I expect to be all fine with (char*)(info + 1), I think it is in other parts of the code anyhow.
| @@ -133,8 +132,8 @@ static str direction_column = str_init("direction"); /* 09 */ | |||
| static str time_us_column = str_init("time_us"); /* 10 */ | |||
| static str totag_column = str_init("totag"); /* 11 */ | |||
|
|
|||
| static str siptrace_info_dlgkey = str_init("__siptrace_info_dlg_key__"); | |||
| static str siptrace_info_avp_str = str_init("$avp(__siptrace_info_avp__)"); | |||
| #define XAVP_TRACE_INFO_NAME "trace_info" | |||
There was a problem hiding this comment.
Would be good to document this as well in the README, to prevent accidential overlapping with existing xavps.
There was a problem hiding this comment.
Sure. Will do this.
|
If no other comments, I am going to merge this one tomorrow. |
|
Thank you |
be passed to DLGCB_CREATED callback. Moreover for transaction tracing
data was allocated in current process memory which would have crashed
if the reply were to be recieved in a different process. With the
current implementation data is allocated in shared memory, all processes
having access to it.
For dialogs data is passed through xavp to dlgcb created. From
there all dialog callbacks are registered and they receive argument
the pointer to siptrace info. For transactions the pointer is passed
as dialog callback parameter.
Pre-Submission Checklist
in
doc/subfolder, the README file is autogenerated)Type Of Change
Checklist:
Description