[Asterisk-code-review] chan sip: Honor support of Symmetric Response (rport) for SI... (asterisk[13])

[Alexander Traud]

Alexander Traud has posted comments on this change.

Change subject: chan_sip: Honor support of Symmetric Response (rport) for SIP requests.
......................................................................


Patch Set 1:

(1 comment)

https://gerrit.asterisk.org/#/c/4027/1/channels/chan_sip.c
File channels/chan_sip.c:

Line 16757: 	    (!ast_test_flag(&peer->flags[0], SIP_NAT_FORCE_RPORT) && !ast_test_flag(&pvt->flags[0], SIP_NAT_RPORT_PRESENT))) {
> Any reason we're not checking for SIP_NAT_FORCE_RPORT in pvt as well?  Ever
parse_register_contact(.) is called at two different places: In one case, SIP_NAT_FORCE_RPORT was copied from pvt to peer for sure. In that case, one could query pvt as well. In the other case, a new peer was created via temp_peer(.) and SIP_NAT_FORCE_RPORT is not set on pvt at all. Therefore, asking peer here, could be *another* issue. I have not checked that. Consequently, the raised question is out of the scope of this change here. If you think you have found an issue, please, file a new, separate report.

git blame channels/chan_sip.c
> 16822 (line of code; shows commit 1a95c9a90)
> quit
git blame 1a95c9a90^ channels/chan_sip.c
> 10900 (line of code; shows commit 48f7381af)
https://github.com/asterisk/asterisk/commit/48f7381af038934ab14cffa25ccbf6e86d5f83ef

The change (which I blame here) was introduced after the decision for peer or pvt. Consequently, this is not related. One could argue that SIP_NAT_RPORT_PRESENT should be copied over from pvt to peer as well (issue report ASTERISK-26438 contains such a patch as alternative example). In that case, that line of code could be left alone and peer is queried for both flags (not pvt). However, a peer is never asked for SIP_NAT_RPORT_PRESENT except here. Therefore, I went for this approach here.


-- 
To view, visit https://gerrit.asterisk.org/4027
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If6e7891848aaf96666dee5305695f7c6667cd5a6
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Alexander Traud <pabstraud at compuserve.com>
Gerrit-Reviewer: Alexander Traud <pabstraud at compuserve.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-HasComments: Yes