[kamailio/kamailio] siptrace: use xavps to pass data for the duration of transaction/dialog (#1963)
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.
<!-- Kamailio Pull Request Template -->
<!-- IMPORTANT: - for detailed contributing guidelines, read: https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md - pull requests must be done to master branch, unless they are backports of fixes from master branch to a stable branch - backports to stable branches must be done with 'git cherry-pick -x ...' - code is contributed under BSD for core and main components (tm, sl, auth, tls) - code is contributed GPLv2 or a compatible license for the other components - GPL code is contributed with OpenSSL licensing exception -->
#### Pre-Submission Checklist <!-- Go over all points below, and after creating the PR, tick all the checkboxes that apply --> <!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines from above--> <!-- If you're unsure about any of these, don't hesitate to ask on sr-dev mailing list --> - [ ] Commit message has the format required by CONTRIBUTING guide - [ ] Commits are split per component (core, individual modules, libs, utils, ...) - [ ] Each component has a single commit (if not, squash them into one commit) - [ ] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change - [ ] Small bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [ ] PR should be backported to stable branches - [ ] Tested changes locally - [ ] Related to issue #1955
#### Description <!-- Describe your changes in detail -->
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/1963
-- Commit Summary --
* siptrace: use xavps to pass data for the duration of transaction/dialog
-- File Changes --
M src/modules/siptrace/siptrace.c (238)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/1963.patch https://github.com/kamailio/kamailio/pull/1963.diff
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 126cfa51be8718082e20e268cf14d0649c60b17d) 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.
@ionutionita92 pushed 1 commit.
fdc627fc0229b6a3ca84fdbdc81823d6108c4f99 siptrace: avoid flooding logs with unnecessary errors on common scenarios
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: * for transactions - no avps is used pointer is passed only using tm callbacks; * for dialogs - as you're saying, dialog lifetime >> tranasaction, so i'm using DLGCB_TERMINATED free function to free the pointer; i'm using the avps only to pass data from sip_trace function to DLGCB_CREATED;
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.
henningw commented on this pull request.
Thank you, great that you could get rid of the serialization/de-serialization code. I had only two small remarks to the pull request. I think it can be merged, you can work on that also directly in the repository.
/* 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);
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?
@@ -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"
Would be good to document this as well in the README, to prevent accidential overlapping with existing xavps.
miconda commented on this pull request.
/* 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);
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.
ionutionita92 commented on this pull request.
@@ -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"
Sure. Will do this.
@ionutionita92 pushed 1 commit.
e29e40ea42632c45f0dd716a29d7da8ba84d2c95 siptrace: document usage of trace_info xavp to prevent overlapping
If no other comments, I am going to merge this one tomorrow.
Merged #1963 into master.
Thank you
-
Daniel-Constantin Mierla -
Henning Westerholt
-
Ionut Ionita