<!-- 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) - [ ] 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) - [ ] 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 - [ ] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description <!-- Describe your changes in detail --> topoh: uses socket IP when no mask_ip is defined
If the parameter mask_ip is not defined the module finds the socket IP and uses that as mask IP for the message. If the socket has an advertised IP it is used, otherwise the socket IP is used.
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3341
-- Commit Summary --
* topoh: uses socket IP when no mask_ip is defined * Merge remote-tracking branch 'upstream/master'
-- File Changes --
M src/modules/topoh/doc/topoh_admin.xml (4) M src/modules/topoh/th_msg.c (55) M src/modules/topoh/th_msg.h (16) M src/modules/topoh/topoh_mod.c (309)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3341.patch https://github.com/kamailio/kamailio/pull/3341.diff
The patch seems to be pretty consistent in size, it will need some time to be reviewed.
@dilyanpalauzov commented on this pull request.
@@ -92,11 +92,13 @@ modparam("topoh", "mask_key", "some secret here")
SIP URIs. Can be any IP address, even a private-space or non-existing IP address (e.g., 192.168.1.1, 127.0.0.2), including the SIP server address, but must not be an address potentially used by clients. + If not set, the advertised IP of the, incoming or outgoing, socket is used.
The comma is not properly used.
@TorPetterson pushed 1 commit.
1314e5db3db6a367831c82fd5496952d3e7a2926 Update topoh_admin.xml
@TorPetterson commented on this pull request.
@@ -92,11 +92,13 @@ modparam("topoh", "mask_key", "some secret here")
SIP URIs. Can be any IP address, even a private-space or non-existing IP address (e.g., 192.168.1.1, 127.0.0.2), including the SIP server address, but must not be an address potentially used by clients. + If not set, the advertised IP of the, incoming or outgoing, socket is used.
Is the new version better?
@dilyanpalauzov commented on this pull request.
@@ -92,11 +92,13 @@ modparam("topoh", "mask_key", "some secret here")
SIP URIs. Can be any IP address, even a private-space or non-existing IP address (e.g., 192.168.1.1, 127.0.0.2), including the SIP server address, but must not be an address potentially used by clients. + If not set, the advertised IP of the, incoming or outgoing, socket is used.
Yes
This PR is stale because it has been open 6 weeks with no activity. Remove stale label or comment or this will be closed in 2 weeks.
I think I resolved the conflicts but the notice is still there?
@henningw commented on this pull request.
The stale flag got now removed from the bot. I think this is a useful addition, even if it changes the default behaviour of the module. For people want the previous behavior back, they can easily configure 127.0.0.1 manually. I did a quick review, just added two comments regarding memory management in mod_init. Other developers want probably also review it, as its a larger patch.
if(th_build_via_prefix(&th_via_prefix, &th_ip))
+ { + goto error; + } + if(th_build_uri_prefix(&th_uri_prefix, &th_ip)) + { + goto error; + } + } else { + th_socket_hash_table = pkg_malloc(sizeof(struct str_hash_table)); + if(th_socket_hash_table == NULL){ + PKG_MEM_ERROR_FMT("th_socket_hash_table\n"); + goto error; + } + if(str_hash_alloc(th_socket_hash_table, TH_HT_SIZE)) + goto error;
We probably should free the th_socket_hash_table in error block.
}
+ } else { + th_socket_hash_table = pkg_malloc(sizeof(struct str_hash_table)); + if(th_socket_hash_table == NULL){ + PKG_MEM_ERROR_FMT("th_socket_hash_table\n"); + goto error; + } + if(str_hash_alloc(th_socket_hash_table, TH_HT_SIZE)) + goto error; + + str_hash_init(th_socket_hash_table); + if(th_parse_socket_list(*get_sock_info_list(PROTO_UDP)) != 0 || + th_parse_socket_list(*get_sock_info_list(PROTO_TCP)) != 0 || + th_parse_socket_list(*get_sock_info_list(PROTO_TLS)) != 0 || + th_parse_socket_list(*get_sock_info_list(PROTO_SCTP)) !=0) + goto error;
We should probably free the th_socket_hash_table and the str_hash_alloc list in error block.
@TorPetterson pushed 1 commit.
da0dafe402bd20af303932ddbfeb30f2cd253251 merge upstream
@TorPetterson pushed 1 commit.
444ff4bc59b13d44d11d2c4fe23bbcfe4ebc0a2b Free resources in error block
Is there anything else I can to to move this pull request forward?
Thanks for the reminder! I am going to merge it, but then I am going to change back that the default value of th_ip to be again 127.0.0.8, like it was so far. That ensures the current behaviour remains the default and the new one can be activated by setting the modparam to empty string, in this way no behavioural change happens on upgrade to next major version of existing deployments.
Merged #3341 into master.
Thanks!
I have no problem with the config change.
-
Daniel-Constantin Mierla -
github-actions[bot]
-
Henning Westerholt
-
TorPetterson
-
Дилян Палаузов