<!-- 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 --> - [ 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 - [x ] 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 --> - [x ] PR should be backported to stable branches - [x ] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description <!-- Describe your changes in detail --> When cnxcc receives a message wih a registered CID (for example in a re-INVITE) it saves as a new call and this is a problem: - cnxcc store 2 times the same call and it discounts credit for both - when the call is finished, cnxcc only remove one record and the other continues discounting credit. - when credit is finished (of an non active call), all active calls are killed
An example: kamcmd> cnxcc.check_client test id:0,confirmed:no,local_consumed_amount:0.004800,global_consumed_amount:3.126200,local_max_amount:30.000000,global_max_amount:30.000000,call_id:41211f7e0a65b41c4ca7181b18db0f1e@185.47.15.56:5080,start_timestamp:0,inip:1,finp:1,connect:0.000000,cps:0.004800; id:1,confirmed:no,local_consumed_amount:0.004800,global_consumed_amount:3.126200,local_max_amount:30.000000,global_max_amount:30.000000,call_id:41211f7e0a65b41c4ca7181b18db0f1e@185.47.15.56:5080,start_timestamp:0,inip:1,finp:1,connect:0.000000,cps:0.004800; id:2,confirmed:no,local_consumed_amount:0.000200,global_consumed_amount:3.126200,local_max_amount:30.000000,global_max_amount:30.000000,call_id:5e911ab1452cf1a3165af5262b519977@185.47.15.56:5080,start_timestamp:0,inip:1,finp:1,connect:0.000000,cps:0.000200; id:3,confirmed:no,local_consumed_amount:0.000200,global_consumed_amount:3.126200,local_max_amount:30.000000,global_max_amount:30.000000,call_id:5e911ab1452cf1a3165af5262b519977@185.47.15.56:5080,start_timestamp:0,inip:1,finp:1,connect:0.000000,cps:0.000200; id:4,confirmed:no,local_consumed_amount:0.000800,global_consumed_amount:3.126200,local_max_amount:30.000000,global_max_amount:30.000000,call_id:0f44fbc63883e52d72c9f52f49253476@185.47.15.56:5080,start_timestamp:0,inip:1,finp:1,connect:0.000000,cps:0.000800;
- id 0 and id 1 are the same call that has been inserted twice. - id 2 and id 3 are the same call that has been inserted twice. - id 4 is a finished call that has not been removed from the stack and continues discounting credit.
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/2383
-- Commit Summary --
* cnxcc: remove duplicated entries with a same CID value
-- File Changes --
M src/modules/cnxcc/cnxcc_mod.c (50)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/2383.patch https://github.com/kamailio/kamailio/pull/2383.diff
I guess the function was supposed to be called only for the initial INVITE, the one that creates the call.
Overall, I do not use that much the module to have an opinion of this is good to have or someone also wants to charge a call twice (e.g., incoming leg and outgoing leg). The initial developer of the module is not active lately in the project, maybe @linuxmaniac can give his opinion as I saw some commits to this module from him rather recently.
As a further remark, the protection could be good, but if there are cases to charge a call many times, then it should be made a modparam to control the behaviour.
Sure. It supposed to be called only for the initial INVITE but there are a check in the code for that (which log message was fixed yesterday by @linuxmaniac). The problem is that this check only print a message and has no control the insertion of the new record.
I've found the problem in some calls where '100 Trying' takes 1 second and origin sends another INVITE message
The t_precheck_trans() and t_check_trans() should help filtering out such retransmissions.
Anyhow, again, the only concern is about the case when one wants to charge a call twice (or even more), either from same billing account or from different accounts... not sure how the PR affects such cases.
You are right. If you want I can prepare a modparam to control the behaviour. Only one CID o multiples CIDs
AFAICT module just supports one record per call-id since ``__stop_billing()``, that is called via dialog.callback when call is DLGCB_TERMINATED | DLGCB_FAILED | DLGCB_EXPIRED, is just getting one record via ``try_get_call_entry()`` and even more it does not support mixing limits ( money, time or channel )
I've been reviewing the code another time.
There are 3 different functions to control credit, time and channels: __set_max_credit(), __set_max_time() and __set_max_channels(). All of them do the same:
**function __set_max_money()**: credit_data = __get_or_create_credit_data_entry(client_id, CREDIT_MONEY) call = __alloc_new_call_by_time(client_id) // Allocate new call for client __add_call_by_cid(cid, call, CREDIT_MONEY) // Allocate new CID_BY_CLIENT for client
**function __set_max_time()**: credit_data = __get_or_create_credit_data_entry(client_id, CREDIT_TIME) call = __alloc_new_call_by_time(client_id) // Allocate new call for client __add_call_by_cid(cid, call, CREDIT_TIME) // Allocate new CID_BY_CLIENT for client
**function __set_max_channel()**: credit_data = __get_or_create_credit_data_entry(client_id, CREDIT_CHANNEL) call = __alloc_new_call_by_time(client_id) // Allocate new call for client __add_call_by_cid(cid, call, CREDIT_CHANNEL) // Allocate new CID_BY_CLIENT for client
If credit is exhausted, the function **__terminate_all()** is called to kill all active calls for this client. This works fine:
__terminate_all(pointer_client_id) -> ki_terminate_all(client_id) -> terminate_all_calls(credit_data): foreach(call_list) { terminate_call(call) }
On the other hand, when a dialog is finished, the function called is **__stop_billing()**:
__dialog_terminated_callback() -> __stop_billing(callid) -> __delete_call(call, credit_data) -> __free_call(call)
In this case, there is no loop to delete and free all calls for this client with a same CID. Only remove the first match found. If the module has prepared to handle several times the same CID (by client), when dialog is finished, it should check and remove all calls with of this client with a same CID. Currently the module is not working in this way and I'm sure that nobody is using it to charge more than 1 times each CID, because it doesn't work. If anyone is trying to charge a call twice, is having problems because only one call by cid is terminated.
However, if you think that this feature could be important (being able to charge the same call more than once), it will be necessary to fix that, because currently it doesn't work.
@Pepelux pushed 1 commit.
77aec0e628d273629075b300d5a251f4d13a9ea8 cnxcc: readme example fixed [skip ci]
My comments were to be sure it doesn't break scenarios expected to work. If not, then it can be merged from my point of view.
@Pepelux pushed 1 commit.
f16ea4705b9ed06184863ff63e9ad7505b1bc177 cnxcc: fix readme example [skip ci]
@Pepelux pushed 2 commits.
5b94195e3107aa4273205c8c14577d4d0f2ba5e6 cnxcc: some return values do not match in readme and code [skip ci] 2c568178154fcd174e61ea3a5168b15eb3871552 cnxcc: Changing return code since -2 is in use
I've squashed the commits and reworked the commit messages
Closed #2383.
-
Daniel-Constantin Mierla -
Jose Luis Verdeguer
-
Victor Seva