[kamailio/kamailio] TSILO / TM. Lookup and branch creating by contact (location record). (PR #2931)
#### 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 --> - [X] Commit message has the format required by CONTRIBUTING guide - [X] Commits are split per component (core, individual modules, libs, utils, ...) - [X] Each component has a single commit (if not, squash them into one commit) - [X] 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) - [X] 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 - [X] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description
Two separate commits per component: TSILO and TM. Important, this implementation has been tested internally and is in use by us already. Contributions to both TSILO and TM are made so, that they save full back-compatibility and do not affect existing implementation.
---
TSILO: Add support of a lookup and branch creating by contact
Improvement of the TSILO module, which allows to run a lookup using the provided RURI, but only to create a new branch for the Contact, which is set in the currently processed REGISTER. Or optionally a Contact URI value given as a parameter to the function. If the Contact URI for a lookup, is given as a parameter, it is possible to provide it as a pseudo-variable.
Hence it is now possible to append new branch(es) for only UAC(s) getting REGISTERED at the moment of calling 'ts_append_by_contact()'.
Previously it was only possible to call 'ts_append()' and create new branches for all previously present location records (for this specific URI), which was in some circumstsances undersired.
New script functions introduced: - ts_append_by_contact(table, ruri [, contact])
New module functions introduced: - w_ts_append_by_contact2() / ki_ts_append_by_contact() - w_ts_append_by_contact3 / ki_ts_append_by_contact_uri() - ts_append_by_contact() / ts_append_by_contact_to()
Backwards compatibility is saved, the new functionality is not overlapping with the previously existing implementation, hence it only acts when the new script function ts_append_by_contact() is used.
The documentation has been updated accordingly.
---
TM: API improvement, new function introduced: t_append_branch_by_contact()
New module functions introduced: - t_append_branch_by_contact()
This commit introduces a possibility of TM's API to append a branch based on specific location (Contact's URI).
Indeed the 't_append_branch_by_contact()' is a corrected copy of the 't_append_branches()', which instead takes Contact as a parameter of str type and takes care to append a branch only for it.
The separate function has been introduced, in order to save full back-compatibility, and not affect existing implementation.
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/2931
-- Commit Summary --
* <a href="https://github.com/kamailio/kamailio/pull/2931/commits/6bdb1dbdd9726b1b45c09dcc7a8611c94cbde5f2">tsilo: Add support of a lookup and branch creating by contact</a> * <a href="https://github.com/kamailio/kamailio/pull/2931/commits/7d0f21edde6a816a75fd0453f50e8421b750b6bb">tm: API improvement, new function introduced: t_append_branch_by_contact()</a>
-- File Changes --
M src/modules/tm/t_append_branches.c (206) M src/modules/tm/t_append_branches.h (4) M src/modules/tm/tm_load.c (1) M src/modules/tm/tm_load.h (1) M src/modules/tsilo/doc/tsilo.xml (5) M src/modules/tsilo/doc/tsilo_admin.xml (54) M src/modules/tsilo/ts_append.c (116) M src/modules/tsilo/ts_append.h (2) M src/modules/tsilo/tsilo.c (269)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/2931.patch https://github.com/kamailio/kamailio/pull/2931.diff
Thank you for the pull request. Just one comment for the tm extension - this is really a lot of duplicated and non-trivial code for this particular function, which doubles maintenance tasks for bug fixes etc.. I assume the difference between the two functions (t_append_branch, t_append_branch_by_contact) is rather small? I would suggest move the common parts to one new function, and then call it from the old and the new one.
Thank you for the PR, indeed an useful addition. I've read the code and it looks fine (I'll have another read), however I have the same remark as Henning about t_append_branch and t_append_branch_by_contact code sharing.
@henningw @grumvalski thanks for giving an advice. I will prepare an additional commit to improve this part.
@henningw @grumvalski what if we merge functions t_append_branches() and t_append_branch_by_contact() into one? Hence leaving only the t_append_branches().
From what I can see, it seems for now it's only TSILO who directly uses t_append_branches(). Correct me please if I'm wrong.
Proposed approach assumes, that the function signature will be changed, and a function parameter 'contact' of str type will be present. Then depending on if it's empty or not, act accordingly: - If the contact parameter is given, then only a desired location is meant for appending. If not found in the location table, an append will not happen for this AOR. - Otherwise create branches for all existing location records of this particular AOR.
I have prepared the implementation and even tested that locally, it seems to be functioning just fine when either calling TSILO's script function ts_append() and ts_append_by_contact().
Please let me know if you see it reasonable, then I will upload a new commit.
Extending the existing function (as you proposed) is probably a good idea, especially because that TSILO is the only one user of the function, as you also mentioned.
@zenichev pushed 2 commits.
eee5f82f8c6c194659ffda497c89ec7f9213863b tm: merge t_append_branches() and t_append_branch_by_contact() 07e9a8c0caa2798b660a9a5aafa8391b95b999e7 tsilo: Be compliant with changes in TM, now only t_append_branches()
@henningw @grumvalski I have uploaded two additional commits, each one per module (tm and tsilo). Please have a look when you have some time and let me know if there are other remarks.
As I mentioned this code has been tested locally and showed itself working.
Thank you @zenichev. I've given another read at the changes on tsilo and, as for tm changes, I think that there is a lot of duplicated code between ts_append/ts_append_by_contact and ts_append_to/ts_append_by_contact. E.g. the only diffrerence between ts_append_to and ts_append_by_contact_to is just the way tm's t_append branches is called (with or without contact). I think that it could be cleaner to expand the current functions' signature to add an optional contact.
@grumvalski done, I've merged similar functionality in ts_append.c implementation. tsilo.c implementation has been updated accordingly. Can you please check out and see if it can be merged like that? I have tested usage of kamailio's script functions ts_append() and ts_append_by_contact() with a new code, and it worked fine.
P.S.: Previously I had an intention to keep as much back-compatibility as possible to let the new code be not overlapping with previously existing.
---
@henningw TM's part had been updated previously as requested. Could you please have a look? Thank you. (Please remember, it looks like t_append_branches() function is only used by TSILO now).
@zenichev pushed 1 commit.
eb856f88d451241d15d64a19a52a0a626d797239 tsilo: Merge similar functions in ts_append.c
The TM change looks fine to me now. As you mentioned its only used from tsilo module. Lets wait for some feedback from @grumvalski about the other part.
@grumvalski approved this pull request.
@@ -22,7 +22,7 @@
#ifndef _TS_APPEND_H #define _TS_APPEND_H
-int ts_append(struct sip_msg* msg, str *ruri, char *table); -int ts_append_to(struct sip_msg* msg, int tindex, int tlabel, char *table, str *uri); +int ts_append(struct sip_msg* msg, str *ruri, str *contact, char *table);
Any specific reason to have the contact arg inserted between ruri and table args, while in ts_append_to is put at the end?
Looks good to me too, just a small comment on the code. Thank you @zenichev.
@zenichev commented on this pull request.
@@ -22,7 +22,7 @@
#ifndef _TS_APPEND_H #define _TS_APPEND_H
-int ts_append(struct sip_msg* msg, str *ruri, char *table); -int ts_append_to(struct sip_msg* msg, int tindex, int tlabel, char *table, str *uri); +int ts_append(struct sip_msg* msg, str *ruri, str *contact, char *table);
To be precise, the only reason was to keep it just after the ruri parameter. So that in both cases with ts_append() and ts_append_to() I have put it unconsciously just after uri (ruri).
Merged #2931 into master.
-
Donat Zenichev -
Federico Cabiddu
-
Henning Westerholt
-
Victor Seva
-
zenichev