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

[Simon Perreault]

> 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.
> 
> rmudgett wrote:
>     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.

1) I'm not suggesting we re-do the DNS lookup for every STUN query.
2) That would mean the STUN API needs to be fixed. Not fixing an API design issue when you can just causes more issues down the road.
3) Yes the socket can remain open. When the IP address changes, the next call to send() should return an error which can be handled the same way as a timeout condition.
4) stun_purge_socket() is wrong and should be fixed. The STUN code needs to use transaction IDs to associate responses with requests.

It looks like the STUN code needs to be fixed before res_stun_monitor can be implemented correctly.


> 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?
> 
> rmudgett wrote:
>     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.

1) Isn't this premature optimization? Furthermore, one less memory allocation would not fix anything in low memory conditions: you would still run out of memory not very far down the road when other code tries to allocate memory.
2) netsock2 should not perform DNS lookups for address literals. If it does, then it's a bug. Code using netsock2 should not try to detect address literals to avoid DNS lookups since that is netsock2's job.


> 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.
> 
> rmudgett wrote:
>     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.

I forgot the STUN subsystem has not been ported to IPv6. Disregard my initial comment.


- Simon


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


On Nov. 28, 2011, 8:54 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1595/
> -----------------------------------------------------------
> 
> (Updated Nov. 28, 2011, 8:54 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 346389 
>   /branches/1.8/include/asterisk/stun.h 346389 
>   /branches/1.8/main/stun.c 346389 
>   /branches/1.8/res/res_stun_monitor.c 346389 
> 
> 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/20111129/4ab904ab/attachment.htm>