[asterisk-dev] [Code Review] Use the correct IP address in the c and o sdp lines when using an externally mapped IP address

[Tilghman Lesher]

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


I wonder if it might be better to create a function called something like get_effective_ip_address that would do the logic for you of distinguishing which IP address should be embedded in the headers, instead of altering actual addresses?


/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/566/#comment4379>

    Don't forget to add braces to conditionals that you add.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/566/#comment4381>

    Ditto.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/566/#comment4380>

    Ditto.


- Tilghman


On 2010-03-22 11:19:48, ebroad wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/566/
> -----------------------------------------------------------
> 
> (Updated 2010-03-22 11:19:48)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> The issue stems from a call to ast_rtp_instance_get_local_address() in chan_sip.c in get_our_media_address(). ast_rtp_instance_get_local_address() will always return the internal IP address of the Asterisk machine, which could pose a problem when using NAT.
> 
> A simple solution would be to override {v,t}sin->sin_addr in get_our_media_address() with p->ourip.sin_addr.s_addr if it is different(which is what the attached diff does), though I don't know what that might break.
> 
> Edit 1: This patch short-circuits some logic later on in get_our_media_address(). ast_rtp_instance_get_local_address() does exactly as the function name suggests, it gets the local address of the machine the Asterisk is running on, which is bad for NAT setups. Should we be calling this in get_our_media_address() or instead just use p->ourip? or should we try to stick with letting the RTP engine decide, and modifying ast_rtp_instance_get_local_address() to return our externally mapped IP when appropriate, which would break from what the function is supposed to do? Comments anyone? 
> 
> 
> This addresses bug 17044.
>     https://issues.asterisk.org/view.php?id=17044
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 252928 
> 
> Diff: https://reviewboard.asterisk.org/r/566/diff
> 
> 
> Testing
> -------
> 
> Applied to trunk, compiled, fax and voice calls made to and from Asterisk successfully.
> 
> 
> Thanks,
> 
> ebroad
> 
>