20 Sep
2022
20 Sep
'22
1:44 p.m.
<!-- 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
- [ ] 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 -->
- [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 -->
Ported all carrierroute module functions to KEMI API.
Here is an example,
```python3
...
# User location service
def ksr_route_location(self, msg):
if KSR.pv.get("$rm") == "INVITE" and
KSR.carrierroute.cr_user_carrier("$fU", "$fd",
"$avp(carrier)") > 0:
if KSR.pv.get("$rm") == "INVITE" and
KSR.carrierroute.cr_route("$avp(carrier)","$avp(domain)","$rU","$rU","call_id")
> 0:
KSR.info('Routing call via user carrier route\n')
self.ksr_route_relay(msg)
rc = KSR.registrar.lookup("location")
if rc < 0:
KSR.tm.t_newtran()
if rc == -1 or rc == -3:
KSR.sl.send_reply(404, "Not Found")
return -255
elif rc == -2:
KSR.sl.send_reply(405, "Method Not Allowed")
return -255
# when routing via usrloc, log the missed calls also
if KSR.is_INVITE() :
KSR.setflag(FLT_ACCMISSED)
self.ksr_route_relay(msg)
return -255
...
```
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3247
-- Commit Summary --
* KEMI API implementation for carrierroute module
-- File Changes --
M src/modules/carrierroute/carrierroute.c (50)
M src/modules/carrierroute/carrierroute.h (1)
M src/modules/carrierroute/cr_func.c (260)
M src/modules/carrierroute/cr_func.h (63)
A src/modules/carrierroute/cr_kemi.c (199)
A src/modules/carrierroute/cr_kemi.h (119)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3247.patch
https://github.com/kamailio/kamailio/pull/3247.diff
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3247
You are receiving this because you are subscribed to this thread.
Message ID: &lt;kamailio/kamailio/pull/3247(a)github.com&gt;
26 Sep
26 Sep
9:01 a.m.
@mshary pushed 1 commit.
33f6d72929228bd1e1ebb7b35608e9b530b6c346 Merge branch 'kamailio:master' into
master
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/3247/files/e72511b423897f2a415405…
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3247/push/11133831988(a)github.com>
27 Sep
27 Sep
12:03 p.m.
@mshary pushed 1 commit.
598e239a82d99fd693ee4db5b9922b1a88fe8576 KEMI API implementation for carrierroute module
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/3247/files/33f6d72929228bd1e1ebb7…
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3247/push/11148047531(a)github.com>
1:45 p.m.
The implementation looks ok to me since it is convenient for now to keep changes to the
regular cmd export functions low.
The force-pushed appeared because of reverting some merge commit and then changing the
commit message.
I think this is ok for a merge.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3247#issuecomment-1259382193
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3247/c1259382193(a)github.com>
4:14 p.m.
Thanks for the PR. Its a bit verbose with all the explicit error handling, but looks good
to me. Lets wait if there is more feedback and then merge it after some days.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3247#issuecomment-1259572854
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3247/c1259572854(a)github.com>
29 Sep
29 Sep
9:32 p.m.
In my opinion, it does not look right. The kamailio.cfg wrapper function should call the
kemi function, not the other way around.
If it is not an out put script variable, then the parameter should not be provided as
kamailio.cfg pseudo-variable name, but as corresponding value. Practically, the example
above with:
```
KSR.carrierroute.cr_user_carrier("$fU", "$fd", ...
```
Should be:
```
KSR.carrierroute.cr_user_carrier(KSR.kx.get_fuser(), KSR.kx.get_fhost(), ...)
```
Also, looking a bit at the code of the commit, for example:
```
int ki_cr_user_carrier(struct sip_msg * _msg, str *_user,
str *_domain, str *_dstavp) {
// native api already checks if any of required argument is null
// so this is not strictly necessary.
if (!_msg || !_user || !_domain || !_dstavp) {
LM_ERR("missing a required parameter\n");
return -1;
};
return w_cr_load_user_carrier(_msg, _user->s, _domain->s, _dstavp->s);
}
```
The content pointed by `_user->s` is `"$fU"` (considering the example given
in the comment above), and inside `w_cr_load_user_carrier()`:
```
int w_cr_load_user_carrier(struct sip_msg * _msg, char *_user, char *_domain, char
*_dstavp) {
gparam_t *pv_user, *pv_domain, *pv_dstavp;
if (_user == NULL) {
LM_ERR("cannot get the user parameter\n");
return -1;
} else {
pv_user = (gparam_t*)_user;
if (cr_load_user_carrier_fixup((void**)&pv_user, 1) != 0) {
LM_ERR("cannot parse the user parameter\n");
return -1;
}
}
...
```
It is doing fixup (a little bit odd way), but then no free fixup.
Anyhow, this is not the proper way kemi is supposed to work, from the kemi script it
should come direct string (or integer) values, not names of Kamailio-specific
pseudo-variables.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3247#issuecomment-1262724500
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3247/c1262724500(a)github.com>
9:53 p.m.
An example of how a kamailio.cfg-exported function `w_posops_pos_append` is resolving the
parameters with pseudo-variables and calls kemi-exported function `ki_posops_pos_append`
(which gets directly values from the embedded interpreter):
-
https://github.com/kamailio/kamailio/blob/master/src/modules/posops/posops_…
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3247#issuecomment-1262743095
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3247/c1262743095(a)github.com>
11:40 p.m.
I am bit lost on how the code execution flow works for KEMI. My understanding was that all
KEMI methods are exported by `sr_kemi_t` structure array, which specifies exported
function names, their internal implementation function name and its arguments either as
string `SR_KEMIP_STR` or integer `SR_KEMIP_INT`.
Each exported function (e.g. `cr_route`) calls internal implementation function (e.g.
`ki_cr_route5`), which then checks arguments and then either do the actual implementation
or calls up a wrapper function (e.g. `w_cr_do_route`) which does some formatting on input
arguments (e.g. `cr_route_fixup`) and then pass it to an existing implementation (e.g.
`cr_do_route`). The output arguments control flow is in the opposite direction. Also since
there is no free fixup in existing implementation of `cmd_export_t`, so I didn't
created or called-in any for KEMI.
```
static cmd_export_t cmds[]={
{"cr_user_carrier", (cmd_function)cr_load_user_carrier, 3,
cr_load_user_carrier_fixup, 0, REQUEST_ROUTE | FAILURE_ROUTE },
{"cr_route", (cmd_function)cr_route5, 5,
cr_route_fixup, 0, REQUEST_ROUTE | FAILURE_ROUTE },
{"cr_route", (cmd_function)cr_route, 6,
cr_route_fixup, 0, REQUEST_ROUTE | FAILURE_ROUTE },
{"cr_nofallback_route",(cmd_function)cr_nofallback_route5, 5,
cr_route_fixup, 0, REQUEST_ROUTE | FAILURE_ROUTE },
{"cr_nofallback_route",(cmd_function)cr_nofallback_route, 6,
cr_route_fixup, 0, REQUEST_ROUTE | FAILURE_ROUTE },
{"cr_next_domain", (cmd_function)cr_load_next_domain, 6,
cr_load_next_domain_fixup, 0, REQUEST_ROUTE | FAILURE_ROUTE },
{0, 0, 0, 0, 0, 0}
};
```
On the python side, my example just shows that whatever we pass as argument (whether `$fU`
or `KSR.kx.get_fuser()` or `"any-string-value"`), the module function will
accept and take care of it (i.e. fetch the actual string value internally), this is
already tested in my lab environment.
However, @miconda has suggested the completely different approach. Can you please point me
to any existing howto or explain a bit more on how to write KEMI implementation of an
existing module function? The example you provide at,
https://github.com/kamailio/kamailio/blob/master/src/modules/posops/posops_…
is very confusing and does not give any hints on how the flow actually works.
Thank you.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3247#issuecomment-1262845427
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3247/c1262845427(a)github.com>
30 Sep
30 Sep
11:57 a.m.
The export of the functions to KEMI is done ok via `sr_kemi_t` structure array and
`mod_register()`, but the implementation is not the right approach. In the KEMI script
(e.g., python, lua, ...) should not be provided parameters with names of
kamailio.cfg-variables, but string/int values computed from expressions in that language.
The fixup-free functions are not *required* for the functions exported to kamailio.cfg,
because they are executed only once, at start-up, therefore is no risk of a memory leak if
fixup-function allocates memory. But if it is called at run time during SIP packets
processing, then it leaks memory. That's why `KSR.x.modf()` is considered *unsafe*.
Actually, what you implemented is more like `KSR.x.modf()`, but without using fixup-free.
I am not familiar at all with carrierroute module, but from C language approach, I pushed
three commits to give an example of exporting `cr_user_carrier()` to KEMI. Only the last
one (1e3f1886ce) is relevant for KEMI exporting, the other two were to prepare (and
somehow fix) the kamailio.cfg-exported function and update the docs for it because it also
made it more flexible in terms of what is accepted as 3rd parameter -- the commits are:
e84c1947b3 and c32ce4d776 .
@lbalaceanu: with the commit e84c1947b3 I tried to fix an improper approach of having a
variable name as parameter, before the `fixup_spve_null()` was used, which is for dynamic
strings (string that can combine static parts with variable names, like
`sip:$rU@$avp(newdomain)`), relying on some hack to access first field expected to be a
variable name, but that is not ensured.
There is a dedicated fixup for single variable name (`fixup_pvar_null()`), which is more
appropriate in this case. With this change, the parameter can be any writable variable,
which includes AVPs, so backward compatibility is ensured. Further restrictions can be
added to allow only AVP by checking `((pv_spec_t *)(*param))->type==PVT_AVP` inside
`cr_load_user_carrier_fixup()` and similar in the newly exported KEMI function
`ki_cr_load_user_carrier()`.
If the changes I made are considered not ok, then they can be reverted, it's no
problem for me, I coded to give a practical example and I was too lazy to go via a
temporary branch considering it is the right approach to implement functions exported to
KEMI.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3247#issuecomment-1263363580
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3247/c1263363580(a)github.com>
12:10 p.m.
@mshary: commenting on your statement:
On the python side, my example just shows that
whatever we pass as argument (whether $fU or KSR.kx.get_fuser() or
"any-string-value"), the module function will accept and take care of it (i.e.
fetch the actual string value internally), this is already tested in my lab environment.
The issue is you try to leverage variables specific for kamailio.cfg-language inside
another programming language, going via kamailio.cfg fixup-mechanism, which is usually
done only at startup to optimize for runtime and many of them allocate memory, which has
to be freed every time if used repeatedly.
It is also some performance penalty to parse every time the parameter for kamailio.cfg
variables, evaluate them, build the evaluated parameter value, use it and the free. Case
by case, this could be desired, but it should not be the common approach.
Further more, some scripting languages use similar notation with `$name` for variables,
like Ruby, and the functions exported by KEMI must to conflict with them -- like in the
next basic example (taken from the internet, not showing a kemi function, but trying to
make the point):
```
#!/usr/bin/ruby
$ru = 10
class Class1
def print_global
puts "Global variable in Class1 is #$ru"
end
end
```
Hoping that makes more sense what I tried to explain.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3247#issuecomment-1263377228
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3247/c1263377228(a)github.com>
12:17 p.m.
@miconda, thank you for the comments/ commits, I am looking into them.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3247#issuecomment-1263385817
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3247/c1263385817(a)github.com>
1:07 p.m.
Worth to add that I haven't done any test, only ensured it compiles.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3247#issuecomment-1263436373
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3247/c1263436373(a)github.com>
3 Oct
3 Oct
7:16 p.m.
Thank you for detailed info. Let me check and update my code.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3247#issuecomment-1265772615
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3247/c1265772615(a)github.com>
10 Oct
10 Oct
10:05 a.m.
@mshary pushed 0 commits.
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/3247/files/78df7a9768c404bcd8a813…
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3247/push/11276878506(a)github.com>
10:05 a.m.
Closed #3247.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3247#event-7551738304
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3247/issue_event/7551738304(a)github.com>
781
days inactive
801
days old
14 comments
4 participants
participants (4)
-
Daniel-Constantin Mierla -
Henning Westerholt
-
Lucian Balaceanu
-
Muhammad Shahzad Shafi