[kamailio/kamailio] TSILO / TM. Lookup and branch creating by contact (location record). (PR #2931)
12 Nov
2021
12 Nov
'21
11:25 a.m.
#### 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/6bdb1dbdd…uot;>tsilo:
Add support of a lookup and branch creating by contact</a>
* <a
href="https://github.com/kamailio/kamailio/pull/2931/commits/7d0f21edd…b">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
--
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/2931
13 Nov
13 Nov
3:48 p.m.
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.
--
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/2931#issuecomment-968133824
14 Nov
14 Nov
1:12 a.m.
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.
--
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/2931#issuecomment-968213929
10:02 a.m.
@henningw @grumvalski thanks for giving an advice. I will prepare an additional commit to
improve this part.
--
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/2931#issuecomment-968307116
4:26 p.m.
@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.
--
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/2931#issuecomment-968366470
15 Nov
15 Nov
4:31 a.m.
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.
--
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/2931#issuecomment-968699501
4:55 a.m.
@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()
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/kamailio/kamailio/pull/2931/files/7d0f21edde6a816a75fd04…
7:01 a.m.
@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.
--
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/2931#issuecomment-968840388
16 Nov
16 Nov
3:37 a.m.
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.
--
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/2931#issuecomment-970043135
17 Nov
17 Nov
9:49 a.m.
@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).
--
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/2931#issuecomment-971654006
9:50 a.m.
@zenichev pushed 1 commit.
eb856f88d451241d15d64a19a52a0a626d797239 tsilo: Merge similar functions in ts_append.c
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/kamailio/kamailio/pull/2931/files/07e9a8c0caa2798b660a9a…
10:52 a.m.
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.
--
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/2931#issuecomment-971712885
18 Nov
18 Nov
2:03 a.m.
@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?
--
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/2931#pullrequestreview-809532675
2:05 a.m.
Looks good to me too, just a small comment on the code. Thank you @zenichev.
--
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/2931#issuecomment-972592549
2:41 a.m.
@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).
--
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/2931#discussion_r751973926
19 Nov
19 Nov
3:19 a.m.
Merged #2931 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/2931#event-5643739168
1064
days inactive
1071
days old
15 comments
5 participants
participants (5)
-
Donat Zenichev -
Federico Cabiddu
-
Henning Westerholt
-
Victor Seva
-
zenichev