[asterisk-dev] [Code Review]: Don't keep the STUN socket open between STUN monitor checks.

[rmudgett]

> On Nov. 24, 2011, 7:20 a.m., Simon Perreault wrote:
> > /branches/1.8/res/res_stun_monitor.c, line 56
> > <https://reviewboard.asterisk.org/r/1595/diff/3/?file=21848#file21848line56>
> >
> >     Why do we need to keep the server address in memory? (Keeping DNS resolution results in memory is an anti-pattern.)
> >     
> >     It seems to me that we can just call connect() on the STUN socket and forget about the server's address. When we create the socket, we resolve the host name, create the socket, connect() it, and discard the server address. When a STUN request fails or times out, we close() the socket and create a new one, starting with the DNS resolution.

1) We do not want to re-resolve the DNS address every time the external address is polled because the resolved address could change and result in a different external NAT address.
2) The call to ast_stun_request() takes socket and destination address parameters.
3) The socket cannot remain open between calls because our IP address could change.  This is the initial reason for the patch.
4) Keeping the socket open between polls needs the stun_purge_socket() hack.


> On Nov. 24, 2011, 7:20 a.m., Simon Perreault wrote:
> > /branches/1.8/res/res_stun_monitor.c, line 326
> > <https://reviewboard.asterisk.org/r/1595/diff/3/?file=21848#file21848line326>
> >
> >     Why do you need to determine if it's a host name or an address literal? Only netsock2 code should care about that. Why is it important for res_stun_monitor to know that?

1) It avoids a potential memory allocation failure path if the value is an address literal.
2) Like ast_dnsmgr_lookup() it avoids the need to do repeated DNS lookups if the value is an address literal.

ast_sockaddr_parse() is a netsock2 API call.


> On Nov. 24, 2011, 7:20 a.m., Simon Perreault wrote:
> > /branches/1.8/configs/res_stun_monitor.conf.sample, line 25
> > <https://reviewboard.asterisk.org/r/1595/diff/3/?file=21845#file21845line25>
> >
> >     Not clear what the syntax is with IPv6 addresses. Are brackets needed? Always or only with a port? Examples would make it clear I think.

IPv6 support is not needed because STUN like NAT is not very useful for IPv6.

This is a BNF style expression to indicate the acceptable form of the value.  In this case if you really supplied an IPv6 literal address, you must have the square brackets around the IPv6 address because an optional port may be supplied.


- rmudgett


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


On Nov. 23, 2011, 5:42 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1595/
> -----------------------------------------------------------
> 
> (Updated Nov. 23, 2011, 5:42 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Don't keep the STUN socket open between STUN monitor checks.
> 
> Keeping the STUN socket always open will cause the STUN monitor to fail if 
> the STUN server address or our own address changes.  
> 
> * Fix the reverse problem of the STUN server address changing after the 
> initial DNS resolution by using the dnsmgr to refresh the DNS resolution.  
> However, refreshing the DNS resolution may not be desireable for all 
> cases, as a result the stunservermonitor option is added to enable this.  
> 
> * Fix ast_stun_request() return value consistency.
> 
> 
> This addresses bug ASTERISK-18327.
>     https://issues.asterisk.org/jira/browse/ASTERISK-18327
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/configs/res_stun_monitor.conf.sample 346241 
>   /branches/1.8/include/asterisk/stun.h 346241 
>   /branches/1.8/main/stun.c 346241 
>   /branches/1.8/res/res_stun_monitor.c 346241 
> 
> Diff: https://reviewboard.asterisk.org/r/1595/diff
> 
> 
> Testing
> -------
> 
> The STUN request still goes out periodically.
> The STUN server DNS address is refreshed periodically if the new stunservermonitor option is enabled.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

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