[asterisk-dev] [Code Review] Add a SIP nat=auto setting

Thu Feb 9 09:36:02 CST 2012

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

Ship it!

A bunch of minor stuff, and it would be better with bacon, but otherwise looks good. Nicely done.

/trunk/CHANGES
<https://reviewboard.asterisk.org/r/1698/#comment10042>

    Stray semicolon here.

/trunk/CHANGES
<https://reviewboard.asterisk.org/r/1698/#comment10043>

    To be more pedantic, I'd suggest saying 'if Asterisk detects that an incoming SIP request crossed a NAT after being sent by the remote endpoint.'

/trunk/CHANGES
<https://reviewboard.asterisk.org/r/1698/#comment10044>

    Unbalanced quotes here.

/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1698/#comment10045>

    s/clients are/the requesting UA is/

    There is only one UA in the mix here, so using a plural seems misleading.

    What does 'later' mean in this context? The handling of 'auto' is done right here in this code block.

/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1698/#comment10046>

    Add output of comedia-related options?

/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1698/#comment10047>

    We could add SIP-Comedia here too.

/trunk/channels/sip/config_parser.c
<https://reviewboard.asterisk.org/r/1698/#comment10048>

    Minor typo.

/trunk/channels/sip/config_parser.c
<https://reviewboard.asterisk.org/r/1698/#comment10049>

    I know it didn't do this before, and it's not documented this way, but using ast_false() and ast_true() would be better than direct comparisons against 'no' and 'yes'... just in case someone tries 'on' or 'off' :-)

/trunk/channels/sip/include/config_parser.h
<https://reviewboard.asterisk.org/r/1698/#comment10051>

    Same typo here. It's probably not worth repeating the doxygen docs in both the header and the implementation files.

/trunk/configs/sip.conf.sample
<https://reviewboard.asterisk.org/r/1698/#comment10053>

    s/ouside of/other than/

/trunk/configs/sip.conf.sample
<https://reviewboard.asterisk.org/r/1698/#comment10054>

    s/therefor/therefore/
    s/it's/its/

/trunk/configs/sip.conf.sample
<https://reviewboard.asterisk.org/r/1698/#comment10056>

    Suggest changing all references to port in this block to 'address/port', since both are taken into account.

/trunk/configs/sip.conf.sample
<https://reviewboard.asterisk.org/r/1698/#comment10057>

    Same thing here... port should become 'address/port'.

- Kevin

On Feb. 8, 2012, 11:34 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1698/
> -----------------------------------------------------------
> 
> (Updated Feb. 8, 2012, 11:34 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> First, this patch as originally posted is from JIRA user pedro-garcia. It has been in JIRA for a long while, and has finally come up to be considered. There were some changes that I wanted to made to the original patch, so I tried contacting the author to get them to put the patch on reviewboard, but after a week I got no response. So, I'm putting the original patch up, then will immediately add a review with my changes. Many thanks to pedro-garcia for his contribution.
> 
> From the JIRA issue:
> 
> I have some devices in the following scenario:
> 
> Asterisk server with public IP address
> Mobile devices (clients):
> 
> When in internal network, no NAT between the client and the server
> When in "roaming" (i.e. a Hotel with WiFi), the client is behing a NAT
> When in 3G, operator transparent sip proxy so it looks as no NAT, but does not support symmetric RTP.
> Sometime, the device gets a public IP with no NAT at all.
> No NAT setting available in asterisk works for all these scenarios at the same time, and I can not request the user to activate different accounts depending on its location.
> 
> I have added a new NAT setting (nat=auto) to the current ones. When set, chan_sip auto detects from the Via header, the recv sockaddr, and the rport setting if the client is behind a NAT.
> 
> It also adds to cli interface results (sip show peer/s) info on this (so now you could see "N" for NAT and nothing for no NAT as before, "a" for auto detect no NAT, and "A" for autodetect NAT.
> 
> 
> This addresses bug ASTERISK-17860.
>     https://issues.asterisk.org/jira/browse/ASTERISK-17860
> 
> 
> Diffs
> -----
> 
>   /trunk/configs/sip.conf.sample 354465 
>   /trunk/channels/sip/config_parser.c 354465 
>   /trunk/channels/sip/include/config_parser.h 354465 
>   /trunk/channels/sip/include/sip.h 354465 
>   /trunk/channels/sip/include/sip_utils.h 354465 
>   /trunk/channels/sip/utils.c PRE-CREATION 
>   /trunk/CHANGES 354465 
>   /trunk/channels/chan_sip.c 354465 
> 
> Diff: https://reviewboard.asterisk.org/r/1698/diff
> 
> 
> Testing
> -------
> 
> Lots of reloads with changing values, and registry natted and un-natted phones. I also set the nat_supertest in testsuite to run with nat=auto to make sure that it responded the same with existing and non-existing peers.
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120209/b43f393b/attachment-0001.htm>