[asterisk-dev] [Code Review] res_stun_monitor module

[Russell Bryant]

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


I haven't gone through all of the new res_stun_monitor module itself yet


/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/854/#comment5620>

    no need to initialize to NULL, that happens automagically (and it is to your benefit not to supply it)



/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/854/#comment5622>

    I don't think inlining this or the unsubscribe function is beneficial



/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/854/#comment5621>

    This last argument needs to be AST_EVENT_IE_END



/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/854/#comment5624>

    Use ast_debug().



/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/854/#comment5623>

    Please add curly braces with your loop traversal.
    
    Also, you might want to consider queueing this up as a task for the IAX2 thread pool to process.  As it is written, this blocks the internal event distribution thread while sending out registrations.



/trunk/channels/chan_iax2.c
<https://reviewboard.asterisk.org/r/854/#comment5625>

    You need to handle a default value for this for when the option is not specified.
    
    For example, what if someone turns it on, removes the option from the conf file, and then reloads?



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/854/#comment5626>

    don't init to NULL



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/854/#comment5627>

    s/NULL/AST_EVENT_IE_END/



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/854/#comment5629>

    Consider making this not block event distribution



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/854/#comment5628>

    ast_debug()



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/854/#comment5630>

    Need to handle when this option is not specified at all



/trunk/configs/iax.conf.sample
<https://reviewboard.asterisk.org/r/854/#comment5631>

    I think the fact that it generates an event that the module subscribes to is an internal implementation detail that doesn't need to be in the sample configuration file.



/trunk/configs/iax.conf.sample
<https://reviewboard.asterisk.org/r/854/#comment5632>

    s/affect/effect/



/trunk/configs/res_stun_monitor.conf.sample
<https://reviewboard.asterisk.org/r/854/#comment5633>

    agreed with a previous comment that this option may not be necessary at all



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

    same comment as for iax.conf.sample



/trunk/res/res_stun_monitor.c
<https://reviewboard.asterisk.org/r/854/#comment5636>

    just 2010



/trunk/res/res_stun_monitor.c
<https://reviewboard.asterisk.org/r/854/#comment5637>

    static const char foo[]



/trunk/res/res_stun_monitor.c
<https://reviewboard.asterisk.org/r/854/#comment5638>

    You can make this an anonymous struct.  You don't use "stun_monitor_args" anywhere.



/trunk/res/res_stun_monitor.c
<https://reviewboard.asterisk.org/r/854/#comment5639>

    It looks like you could convert some of this struct to bit fields
    
    unsigned int foo:1;



/trunk/res/res_stun_monitor.c
<https://reviewboard.asterisk.org/r/854/#comment5640>

    < 0, or == -1?  Would it ever be anything else?



/trunk/res/res_stun_monitor.c
<https://reviewboard.asterisk.org/r/854/#comment5641>

    ensure resources are freed/destroyed in this error case



/trunk/res/res_stun_monitor.c
<https://reviewboard.asterisk.org/r/854/#comment5642>

    remove this line


- Russell


On 2010-08-11 11:46:41, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/854/
> -----------------------------------------------------------
> 
> (Updated 2010-08-11 11:46:41)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> The res_stun_monitor module monitors the perceived external ip/port provided by a STUN server and sends out an event when that perceived ip/port changes in any way.  This patch adds chan_sip and chan_iax as listeners to this event.  Both of these channel drivers react to the event by regenerating all outbound registers in order to alert the endpoints being registered with to the new address location.  This feature's primary use case is when Asterisk is behind a NAT in which the external ip address is dynamic or the port mappings of the NAT device are subject to change.
> 
> I will update the CHANGES file before committing this.  That is not included within this review though.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_iax2.c 281758 
>   /trunk/channels/chan_sip.c 281758 
>   /trunk/configs/iax.conf.sample 281758 
>   /trunk/configs/res_stun_monitor.conf.sample PRE-CREATION 
>   /trunk/configs/sip.conf.sample 281758 
>   /trunk/include/asterisk/event_defs.h 281758 
>   /trunk/res/res_stun_monitor.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/854/diff
> 
> 
> Testing
> -------
> 
> I tested all the res_stun_monitor.conf options to verify functionality.
> I tested the channel driver's event listening by triggering an address change and verifying registrations are sent via wireshark.
> 
> 
> Thanks,
> 
> David
> 
>