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

Thu Feb 9 11:59:04 CST 2012

> On Feb. 9, 2012, 9:36 a.m., Kevin Fleming wrote:
> > /trunk/CHANGES, line 64
> > <https://reviewboard.asterisk.org/r/1698/diff/3/?file=23995#file23995line64>
> >
> >     Stray semicolon here.

Using a semicolon as a separator inside parenthetical commas is acceptable. With that said, since there are only two options between the comma, I will remove the semicolon. :-)

> On Feb. 9, 2012, 9:36 a.m., Kevin Fleming wrote:
> > /trunk/CHANGES, line 66
> > <https://reviewboard.asterisk.org/r/1698/diff/3/?file=23995#file23995line66>
> >
> >     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.'

Changed.

> On Feb. 9, 2012, 9:36 a.m., Kevin Fleming wrote:
> > /trunk/CHANGES, line 133
> > <https://reviewboard.asterisk.org/r/1698/diff/3/?file=23995#file23995line133>
> >
> >     Unbalanced quotes here.

Fixed.

> On Feb. 9, 2012, 9:36 a.m., Kevin Fleming wrote:
> > /trunk/channels/chan_sip.c, line 16372
> > <https://reviewboard.asterisk.org/r/1698/diff/3/?file=23996#file23996line16372>
> >
> >     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.

Changed. Later means when the flags are copied from the the pvt to the peer where they are actually used.

> On Feb. 9, 2012, 9:36 a.m., Kevin Fleming wrote:
> > /trunk/channels/chan_sip.c, line 17508
> > <https://reviewboard.asterisk.org/r/1698/diff/3/?file=23996#file23996line17508>
> >
> >     Add output of comedia-related options?

Added.

> On Feb. 9, 2012, 9:36 a.m., Kevin Fleming wrote:
> > /trunk/channels/chan_sip.c, line 18304
> > <https://reviewboard.asterisk.org/r/1698/diff/3/?file=23996#file23996line18304>
> >
> >     We could add SIP-Comedia here too.

Added.

> On Feb. 9, 2012, 9:36 a.m., Kevin Fleming wrote:
> > /trunk/channels/sip/config_parser.c, line 769
> > <https://reviewboard.asterisk.org/r/1698/diff/3/?file=23997#file23997line769>
> >
> >     Minor typo.

Fixed.

> On Feb. 9, 2012, 9:36 a.m., Kevin Fleming wrote:
> > /trunk/channels/sip/config_parser.c, line 794
> > <https://reviewboard.asterisk.org/r/1698/diff/3/?file=23997#file23997line794>
> >
> >     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' :-)

Changed.

> On Feb. 9, 2012, 9:36 a.m., Kevin Fleming wrote:
> > /trunk/channels/sip/include/config_parser.h, line 47
> > <https://reviewboard.asterisk.org/r/1698/diff/3/?file=23998#file23998line47>
> >
> >     Same typo here. It's probably not worth repeating the doxygen docs in both the header and the implementation files.

Fixed.

> On Feb. 9, 2012, 9:36 a.m., Kevin Fleming wrote:
> > /trunk/configs/sip.conf.sample, line 854
> > <https://reviewboard.asterisk.org/r/1698/diff/3/?file=24002#file24002line854>
> >
> >     Suggest changing all references to port in this block to 'address/port', since both are taken into account.

Changed.

> On Feb. 9, 2012, 9:36 a.m., Kevin Fleming wrote:
> > /trunk/configs/sip.conf.sample, line 861
> > <https://reviewboard.asterisk.org/r/1698/diff/3/?file=24002#file24002line861>
> >
> >     Same thing here... port should become 'address/port'.

Changed.

- Terry

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

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/789f004b/attachment-0001.htm>