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

[Joshua Colp]

> On March 8, 2013, 4:59 p.m., Mark Michelson wrote:
> > /team/group/pimp_my_sip/include/asterisk/res_sip_session.h, lines 196-201
> > <https://reviewboard.asterisk.org/r/2363/diff/3/?file=33844#file33844line196>
> >
> >     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.

I've changed the callback name to be specific that it is for changing the media stream. What that actually means I've left up to the SDP handler itself. As for using this to change a direct media address... I suppose you could, but you still need to do the codec part as well.


> On March 8, 2013, 4:59 p.m., Mark Michelson wrote:
> > /team/group/pimp_my_sip/res/res_sip_sdp_audio.c, line 177
> > <https://reviewboard.asterisk.org/r/2363/diff/3/?file=33849#file33849line177>
> >
> >     You don't have to, but you could use RAII_VAR for this allocation.

I didn't bother with this. Doesn't really gain you anything.


- Joshua


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


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/20130311/f7d0610e/attachment.htm>