[asterisk-dev] [Code Review] Pimp SIP Nat

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2363/#review8021
-----------------------------------------------------------



/team/group/pimp_my_sip/include/asterisk/res_sip_session.h
<https://reviewboard.asterisk.org/r/2363/#comment15377>

    This is a good change, but I don't think it's all the way to what it should be.
    
    I think you should go one of two ways with this callback
    
    1) If this callback is solely intended to be used in order to change media addresses within SDPs, then the callback name should reflect that more. In addition, the callback should contain parameters for what addresses should be changed and what they should be changed to. What's interesting about this is that the same callback could potentially be used for changing the media addresses to use a direct media address.
    
    2) If this callback could potentially be used for other SDP amendments, then there should be a parameter that tells the SDP handler why this function is being called.  This way, if it gets called with, say, AST_SIP_SDP_CHANGE_NAT, then it knows that it's being called to change the localnet addresses to external addresses. Some other value would mean something else should be changed.



/team/group/pimp_my_sip/res/res_sip_sdp_audio.c
<https://reviewboard.asterisk.org/r/2363/#comment15378>

    You don't have to, but you could use RAII_VAR for this allocation.


- Mark


On March 6, 2013, 6:04 a.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2363/
> -----------------------------------------------------------
> 
> (Updated March 6, 2013, 6:04 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This change adds a new module, res_sip_nat, which performs rewriting of outgoing messages for cases where address information needs to be replaced when Asterisk is behind NAT. It also performs rewriting of incoming messages for cases where the remote party may be behind NAT. This change also adds ICE support.
> 
> 
> Diffs
> -----
> 
>   /team/group/pimp_my_sip/include/asterisk/res_sip.h 382479 
>   /team/group/pimp_my_sip/include/asterisk/res_sip_session.h 382479 
>   /team/group/pimp_my_sip/res/res_sip/config_transport.c 382479 
>   /team/group/pimp_my_sip/res/res_sip/sip_configuration.c 382479 
>   /team/group/pimp_my_sip/res/res_sip/sip_distributor.c 382479 
>   /team/group/pimp_my_sip/res/res_sip_nat.c PRE-CREATION 
>   /team/group/pimp_my_sip/res/res_sip_sdp_audio.c 382479 
>   /team/group/pimp_my_sip/res/res_sip_session.c 382479 
> 
> Diff: https://reviewboard.asterisk.org/r/2363/diff
> 
> 
> Testing
> -------
> 
> Tested Asterisk behind NAT with proper configuration, as well as devices behind NAT. Also tested ICE negotiation.
> 
> 
> Thanks,
> 
> Joshua
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130308/757803d2/attachment.htm>