1 Feb
2019
1 Feb
'19
5:44 p.m.
The setting "keep_proxy_rr" will add the Record-Route headers added by the
proxy to the route_set stored in the dialog. When in use, sending locally
generated in-dialog requests will loop back to the proxy with a proper
Record-Route header, including any parameters.
<!-- 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 on 5.1, compile tested on master
- [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description
<!-- Describe your changes in detail -->
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/1832
-- Commit Summary --
* dialog: Add setting to loop BYE through proxy
-- File Changes --
M src/modules/dialog/dialog.c (9)
M src/modules/dialog/dlg_handlers.c (29)
M src/modules/dialog/dlg_handlers.h (2)
M src/modules/dialog/dlg_hash.c (82)
M src/modules/dialog/dlg_hash.h (9)
M src/modules/dialog/doc/dialog_admin.xml (36)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/1832.patch
https://github.com/kamailio/kamailio/pull/1832.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/1832
12 Feb
12 Feb
2:08 a.m.
@gaaf Thank you, can you add a bit here about the targeted use case for this
functionality?
--
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/1832#issuecomment-462494657
3:20 a.m.
@gaaf - I see that dlg_update_rr_set() is used only for DLG_CALLER_LEG, no need to do it
for CALLEE as well?
Having an option to update Record-Route sets within dialog can be useful, I think at this
moment they are stored only during the call setup.
On the other hand, for what you try to achieve, I wonder if it would not be simpler to use
somehow the caller/callee sockets fields, they are already stored inside the dialog record
and they should overlap in address, port and transport with local record-route. So based
on modparam, the local requests are sent to the caller/callee socket, with top record
route built at storage time or at send time (store vs cpu question here, like none that
relevant to decide for one or the other option) -- this just as an idea, I haven't
really analyzed if it is a simpler solution, I just thought that those addresses are
already stored.
--
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/1832#issuecomment-462518197
2:34 p.m.
@henningw: In the current situation, the local Record-Route headers are stripped before
they are stored in the dialog module. If a request is sent by the dialog module (forced
BYE for example) it completely bypasses the routing script as it will be sent to the
remote parties directly. With this PR the local Record-Route headers will not be stripped
so the request will be looped back to the proxy as the first Route header is now the proxy
itself. This causes the request to follow the routing script as if it was sent by one of
the parties in the call and all processing that would normally be done for an
"external" request would also be done for this request.
@miconda:
- The callee is handled only when the dialog is confirmed. As there is no previous RR set,
no update is needed. The caller's leg is preliminary set when the request is handled,
but the final RR set is only known when the dialog gets confirmed (dependent on the
winning branch), hence the update is needed.
- Changing the RR in-dialog is not the intention of this PR.
- The sockets don't store (record-)route parameters so this would not be enough. As
alternative, maybe using local_route is possible, but it still needs access to RR
parameters to be complete. As this patchset predates the local_route functionality I have
not investigated much into that direction.
In short, this PR makes the dialog module to not strip the local record-route headers,
something I have always considered to be a bug.
--
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/1832#issuecomment-462686226
19 Feb
19 Feb
1:45 a.m.
Thank you for the explanation, understand it now. I will have a brief look to the patch
and comment there if I spot something.
--
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/1832#issuecomment-464871943
2:49 a.m.
henningw commented on this pull request.
From the code change all looks fine to me. I've
added some comments inline about a few things I noticed.
@@ -528,7 +533,7 @@ int dlg_set_leg_info(struct
dlg_cell *dlg, str* tag, str *rr, str *contact,
if(dlg->tag[leg].s)
shm_free(dlg->tag[leg].s);
- dlg->tag[leg].s = (char*)shm_malloc( tag->len + rr->len );
Why you decrease here the shm malloc'ed area, do you change something in the tag as
well?
@@ -269,6 +271,25 @@ int populate_leg_info( struct
dlg_cell *dlg, struct sip_msg *msg,
if (rr_set.s) pkg_free(rr_set.s);
This free is in current git master at the end of this function and seems redundant now.
Maybe it make sense to combine it with the other free in line 290 and move it to the end
of the function?
+ /* skip_recs contains the number of RR's for
the callee */
+ skip_recs -= own_rr;
+ /* Add local RR's to caller's routeset */
+ if( print_rr_body(msg->record_route, &rr_set, DLG_CALLER_LEG,
+ &skip_recs) != 0) {
+ LM_ERR("failed to print route records \n");
+ goto error0;
+ }
+ LM_DBG("updating caller route_set %.*s\n",
+ rr_set.len, rr_set.s);
+ if (dlg_update_rr_set( dlg, DLG_CALLER_LEG, &rr_set)!=0) {
+ LM_ERR("dlg_update_rr_set failed\n");
+ if (rr_set.s) pkg_free(rr_set.s);
+ goto error0;
+ }
+ if (rr_set.s) pkg_free(rr_set.s);
see above
--
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/1832#pullrequestreview-204957921
13 Jul
13 Jul
1:31 a.m.
@gaaf This is apparently still open, I would like to resolve this for the upcoming 5.3 to
actually merge it. I've had a minor few remarks/questions to the code, would be great
if you can have a look to that.
--
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/1832#issuecomment-511023570
15 Jul
15 Jul
6:09 p.m.
gaaf commented on this pull request.
@@ -269,6 +271,25 @@ int populate_leg_info( struct
dlg_cell *dlg, struct sip_msg *msg,
if (rr_set.s) pkg_free(rr_set.s);
print_rr_body() allocates the memory for rrset.s, it is up to the caller to free it. As it
may be allocated twice, it must be freed twice and that must be done before the next call
to print_rr_body(). Each free is in the same scope as the call to print_rr_body(). I fail
to see how these can be combined into one free.
--
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/1832#discussion_r303425969
6:10 p.m.
gaaf commented on this pull request.
+ /* skip_recs contains the number of RR's for
the callee */
+ skip_recs -= own_rr;
+ /* Add local RR's to caller's routeset */
+ if( print_rr_body(msg->record_route, &rr_set, DLG_CALLER_LEG,
+ &skip_recs) != 0) {
+ LM_ERR("failed to print route records \n");
+ goto error0;
+ }
+ LM_DBG("updating caller route_set %.*s\n",
+ rr_set.len, rr_set.s);
+ if (dlg_update_rr_set( dlg, DLG_CALLER_LEG, &rr_set)!=0) {
+ LM_ERR("dlg_update_rr_set failed\n");
+ if (rr_set.s) pkg_free(rr_set.s);
+ goto error0;
+ }
+ if (rr_set.s) pkg_free(rr_set.s);
see above
--
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/1832#discussion_r303426046
6:16 p.m.
gaaf commented on this pull request.
@@ -528,7 +533,7 @@ int dlg_set_leg_info(struct
dlg_cell *dlg, str* tag, str *rr, str *contact,
if(dlg->tag[leg].s)
shm_free(dlg->tag[leg].s);
- dlg->tag[leg].s = (char*)shm_malloc( tag->len + rr->len );
It is not necessarily decreased, it is just realloc'ed to the size of the new tag,
which may be bigger or smaller.
The tag is not "changed" it is stored untouched from the incoming response.
There may be multiple to-tag's in a single dialog as the call may have been forked.
The dialog module does not track individual branches separately, it just stores 1 of them,
we just hope it is the correct one. When storing the tag of the 200 OK, it may be
overwriting a previous tag (from a 1XX response).
--
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/1832#discussion_r303429055
16 Jul
16 Jul
12:13 a.m.
henningw commented on this pull request.
@@ -528,7 +533,7 @@ int dlg_set_leg_info(struct
dlg_cell *dlg, str* tag, str *rr, str *contact,
if(dlg->tag[leg].s)
shm_free(dlg->tag[leg].s);
- dlg->tag[leg].s = (char*)shm_malloc( tag->len + rr->len );
Thank you, understood.
--
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/1832#discussion_r303592384
12:14 a.m.
henningw commented on this pull request.
@@ -269,6 +271,25 @@ int populate_leg_info( struct
dlg_cell *dlg, struct sip_msg *msg,
if (rr_set.s) pkg_free(rr_set.s);
Thanks for the clarification
--
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/1832#discussion_r303592806
12:16 a.m.
Merged #1832 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/1832#event-2483958572
12:16 a.m.
Thank you!
--
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/1832#issuecomment-511533237
1757
days inactive
1921
days old
13 comments
3 participants
participants (3)
-
Alex Hermann -
Daniel-Constantin Mierla
-
Henning Westerholt