[Asterisk-code-review] res pjsip sdp rtp: Control endpoints media address use per c... (asterisk[13])

[Corey Farrell]

Corey Farrell has posted comments on this change. ( https://gerrit.asterisk.org/10431 )

Change subject: res_pjsip_sdp_rtp: Control endpoints media_address use per call basis
......................................................................


Patch Set 3: Code-Review-1

(1 comment)

I'm unsure where but this would need to be documented (more than just the commit message).  If we do go with a magic variable I think it would be better to prefix the name with "PJSIP_" to avoid potentially giving meaning to a variable that someone is already using in generic dialplan.

For the record I'm against creating a magic variable, but I'll leave the final decision to those who are more invested in the PJSIP channel driver.

https://gerrit.asterisk.org/#/c/10431/3/res/res_pjsip_sdp_rtp.c
File res/res_pjsip_sdp_rtp.c:

https://gerrit.asterisk.org/#/c/10431/3/res/res_pjsip_sdp_rtp.c@1254
PS3, Line 1254: 				hostip = ast_sip_get_host_ip_string(session->endpoint->media.rtp.ipv6 ? pj_AF_INET6() : pj_AF_INET());
This does not match the functionality you specified.  The commit message specifies functionality for "yes", "no", or unset.  This code causes the "no" path to be hit for all non-blank values which are not "yes".  So if someone has the value "http://media-server/audio/" this will do the wrong thing.  I think unexpected values (neither "yes" nor "no") should follow the existing functionality.



-- 
To view, visit https://gerrit.asterisk.org/10431
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I852adbee9f8b034ec332fbe4e1b6692cb2939518
Gerrit-Change-Number: 10431
Gerrit-PatchSet: 3
Gerrit-Owner: Salah Ahmed <txrubel at gmail.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2 (1000185)
Gerrit-Comment-Date: Fri, 05 Oct 2018 07:52:06 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20181005/54103de3/attachment.html>