[kamailio/kamailio] siptrace: use xavps to pass data for the duration of transaction/dialog (#1963)
22 May
2019
22 May
'19
5:38 p.m.
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
--
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/1963
23 May
23 May
10:59 a.m.
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.
--
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/1963#issuecomment-495110647
11:28 a.m.
@ionutionita92 pushed 1 commit.
fdc627fc0229b6a3ca84fdbdc81823d6108c4f99 siptrace: avoid flooding logs with unnecessary
errors on common scenarios
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/kamailio/kamailio/pull/1963/files/df349aad7db5613d0d0677…
11:29 a.m.
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.
--
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/1963#issuecomment-495119647
12:53 p.m.
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.
--
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/1963#issuecomment-495150603
24 May
24 May
12:04 a.m.
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.
--
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/1963#pullrequestreview-241442745
11:10 a.m.
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.
--
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/1963#discussion_r287256590
11:12 a.m.
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.
--
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/1963#discussion_r287257267
27 May
27 May
11:09 a.m.
@ionutionita92 pushed 1 commit.
e29e40ea42632c45f0dd716a29d7da8ba84d2c95 siptrace: document usage of trace_info xavp to
prevent overlapping
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/kamailio/kamailio/pull/1963/files/fdc627fc0229b6a3ca84fd…
28 May
28 May
2:28 p.m.
If no other comments, I am going to merge this one tomorrow.
--
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/1963#issuecomment-496476724
10:56 p.m.
Merged #1963 into master.
--
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/1963#event-2372961482
10:56 p.m.
Thank you
--
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/1963#issuecomment-496665220
2035
days inactive
2041
days old
11 comments
3 participants
participants (3)
-
Daniel-Constantin Mierla -
Henning Westerholt
-
Ionut Ionita