<!-- 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 --> - [ ] 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 --> This feature was made half year ago. We would like to have inside location record address of the host where kamailio is installed. However when we installed it behind NAT we got private address in this field. This feature introduces possibility to set a socket string which will be written into location record. It could be public address instead address on which the register request was received. The default value is NULL and it works as usual. The feature was installed on our production servers in April and since that time it works well. I would like to propose this feature to others, because it could be useful. You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/2498
-- Commit Summary --
* registrar: added new parameter sock_addr
-- File Changes --
M src/modules/registrar/doc/registrar_admin.xml (25) M src/modules/registrar/registrar.c (2) M src/modules/registrar/registrar.h (1) M src/modules/registrar/save.c (39)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/2498.patch https://github.com/kamailio/kamailio/pull/2498.diff
This works when listening on a single socket, right?
While that is ok for some use cases, if you want to have the advertised address of a listen socket, then this can be coded in a different way, by saving `socket->useinfo.sock_str` instead of `socket->sock_str`.
If you still want to have a parameter like in your patch, it is fine for me, even it has rather limited usage, but in this case I think it should use a static variable in pack_ci() function, like all the other variables in the function that are used outside, to avoid the pkg_malloc() and the pkg_free(), so declare:
``` static struct socket_info si = {0}; ```
Then have something like:
``` if (sock_addr.len>0) { memset(&si, 0, sizeof(struct socket_info)); si.sock_str = sock_addr; ci.sock = &si; ... ```
Then IF blocks to do `pkg_free(si);` and `pkg_free(ci->sock);` should dissapear, having the changes only inside the pack_ci() function.
Thank you for advise. I will fix tomorrow.
Hi Daniel! I have question about useinfo structure. When I store sock_addr in socket->useinfo.sock_str kamailio crashes. Should I initialize other fields of the structure? The comment against useinfo tells that it is for "details to be used in SIP msg". Actually we do not want to use this address in SIP messages. Is that correct place to assign socket address? May I keep it in socket->sock_str?
Daniel, i have implemented your proposal and found that useinfo doesn't override socket field in db. So I think we need to keep first approach.
The use of `useinfo` field is when you want to store `advertised` address set to `listen` parameter. It is not about using it for the value of the new parameter `sock_addr`.
As I mentioned above, setting the `sock_addr` value via modparm is ok only using kamailio with a single listen address. I find its purpose really limited from that perspective.
The socket field in database location table is important to match a local listen socket (by listen address or advertised address) when phones connect from behind nat.
So, what I meant to store the value from useinfo requires to update other parts of usrloc (or registrar) module to use it when is set (that happens when advertise is in listen value) and if not, store the filed done right now.
@tiglat pushed 1 commit.
970d559fa65194992ac43c6bc1c6d52f951c8d93 registrar: replaced dynamically allocated var with static var
Daniel, please code below
Is that you meant before?
if (_m->rcv.bind_address->useinfo.sock_str.len > 0) { memset(&si, 0, sizeof(struct socket_info)); si.sock_str = _m->rcv.bind_address->useinfo.sock_str; ci.sock = &si;
It works as we expected.
@tiglat pushed 1 commit.
82b9f983c43a4ea6b218e7f1a97febf61d58d008 registrar: socket field in location db will be initialized by advertise address from listen parameter
@tiglat pushed 1 commit.
3e2626ad0bba9fc391af02ef70b94bab4bfa82fc registrar: added additional check socket bind_address for null
Looking at the combined diff against existing code, I think the condition on sock_flag has to be processed first (higher priority), respectively:
``` if (_m->flags&sock_flag) { ```
Because that is a config admin instructing to take the value from a different place. With this PR, that will never work when advertised address is set.
Moreover, using advertised address (the `useinfo`) should be controlled via a modparam, because from sip routing point of view it is more useful to have local listen socket than the public ip. So it needs a new parameter for having this new feature enabled.
@tiglat pushed 1 commit.
8aaf69aa065325ba0cf4015897c6986fe3983a1f registrar: introduced config parameter to enable/disable socket advertised address feature
Thanks! I will squash and merge, but I plan to do some changes to this code to make it a bit more open for extensions in the future (e.g., maybe one wants to store socket name at some point).
Merged #2498 into master.
Renamed the mod param to `sock_mode` and advertised address is stored when this param is set to 1. See docs for more details. If any problem with it, then open a bug in the tracker.
-
Daniel-Constantin Mierla -
Konstantin