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

[rmudgett]

> On Nov. 23, 2011, 10:52 a.m., mjordan wrote:
> > Do we also need to update the CHANGES.txt, since this adds a parameter to the res_stun_monitor.conf file?

Will do.


> On Nov. 23, 2011, 10:52 a.m., mjordan wrote:
> > /branches/1.8/res/res_stun_monitor.c, line 288
> > <https://reviewboard.asterisk.org/r/1595/diff/1/?file=21831#file21831line288>
> >
> >     Should it?  That seems a bit odd - if I have in my config file:
> >     
> >     stunaddr =>
> >     
> >     I would expect that to be a configuration error, not a request to not have STUN monitoring.  At the very least, it seems that would make the "stunservermonitor" parameter redundant (also, if blank is acceptable but turns off STUN monitoring, that should be noted in the config file)

If I did not make an empty value disable STUN monitor then there would be no way to disable STUN monitoring short of a reload with no stunaddr listed.  Previously, the STUN monitor was disabled by giving a bogus address.  The dnsmgr API does not indicate if you gave it an unresolvable address so I could not do the same thing without redundant code.

The stunservermonitor parameter enables refreshing the DNS address of the STUN server.  It does not affect if the STUN monitoring is enabled.  Have you a better name?


> On Nov. 23, 2011, 10:52 a.m., mjordan wrote:
> > /branches/1.8/main/stun.c, line 374
> > <https://reviewboard.asterisk.org/r/1595/diff/1/?file=21830#file21830line374>
> >
> >     May want to change the documentation here to say that:
> >     <0 => error
> >     0 => success
> >     1 => timeout (or greater than 0)
> >

Will do.


> On Nov. 23, 2011, 10:52 a.m., mjordan wrote:
> > /branches/1.8/res/res_stun_monitor.c, line 207
> > <https://reviewboard.asterisk.org/r/1595/diff/1/?file=21831#file21831line207>
> >
> >     May as well add the \internal tag on this guy as well

Will do.


> On Nov. 23, 2011, 10:52 a.m., mjordan wrote:
> > /branches/1.8/res/res_stun_monitor.c, lines 273-279
> > <https://reviewboard.asterisk.org/r/1595/diff/1/?file=21831#file21831line273>
> >
> >     This function seems to do a bit more then parsing and setting up the address, since it also disables the STUN monitor, reenables it if it parses correctly, etc.
> >     
> >     If you want all this functionality in one method, I'd rename it to reflect all of its functionality

I'll rename it to setup_stunaddr().


- rmudgett


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


On Nov. 22, 2011, 6:43 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1595/
> -----------------------------------------------------------
> 
> (Updated Nov. 22, 2011, 6:43 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/main/stun.c 345978 
>   /branches/1.8/res/res_stun_monitor.c 345978 
>   /branches/1.8/configs/res_stun_monitor.conf.sample 345978 
> 
> 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/20111123/9b55e830/attachment-0001.htm>