[asterisk-dev] [Code Review] 2584: New SIP Channel Driver - SIP Qualify Support

[Joshua Colp]

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



/team/group/pimp_my_sip/res/res_sip/sip_options.c
<https://reviewboard.asterisk.org/r/2584/#comment17173>

    Turn this into an enum.



/team/group/pimp_my_sip/res/res_sip/sip_options.c
<https://reviewboard.asterisk.org/r/2584/#comment17174>

    I don't know what this really means/is used for by reading the comment.



/team/group/pimp_my_sip/res/res_sip/sip_options.c
<https://reviewboard.asterisk.org/r/2584/#comment17175>

    Yay for using nolock!



/team/group/pimp_my_sip/res/res_sip/sip_options.c
<https://reviewboard.asterisk.org/r/2584/#comment17176>

    These already get initialized to zero.



/team/group/pimp_my_sip/res/res_sip/sip_options.c
<https://reviewboard.asterisk.org/r/2584/#comment17177>

    Can you add a comment explaining this logic for someone casually reading?



/team/group/pimp_my_sip/res/res_sip/sip_options.c
<https://reviewboard.asterisk.org/r/2584/#comment17172>

    As long as we get *any* response back then they are reachable. Actually authenticating can cause useless system load on the remote system.



/team/group/pimp_my_sip/res/res_sip/sip_options.c
<https://reviewboard.asterisk.org/r/2584/#comment17178>

    Tweak this to be proper english ^_^



/team/group/pimp_my_sip/res/res_sip/sip_options.c
<https://reviewboard.asterisk.org/r/2584/#comment17179>

    This isn't safe. You've still got a small race condition where data could be freed in another thread while the scheduled item is executing. While the contact object itself may be valid the data object won't be, and the pointer to contact could end up being garbage.



/team/group/pimp_my_sip/res/res_sip/sip_options.c
<https://reviewboard.asterisk.org/r/2584/#comment17180>

    You could just RAII_VAR this whole thing.



/team/group/pimp_my_sip/res/res_sip/sip_options.c
<https://reviewboard.asterisk.org/r/2584/#comment17186>

    This should be inside of the qualify_frequency check, eitherwise you do it even if no qualify frequency is configured.



/team/group/pimp_my_sip/res/res_sip/sip_options.c
<https://reviewboard.asterisk.org/r/2584/#comment17185>

    If a contact has been updated it's currently because they are re-registering and updating expiration time. I think qualifying/scheduling isn't really needed.



/team/group/pimp_my_sip/res/res_sip/sip_options.c
<https://reviewboard.asterisk.org/r/2584/#comment17184>

    This needlessly allocates, creates, and deletes if contact status is not found. Since the contact is being deleted it doesn't matter if it wasn't found.



/team/group/pimp_my_sip/res/res_sip/sip_options.c
<https://reviewboard.asterisk.org/r/2584/#comment17183>

    Do you think another developer may end up using OBJ_KEY? If not then remove and note its absence, otherwise implement it.



/team/group/pimp_my_sip/res/res_sip/sip_options.c
<https://reviewboard.asterisk.org/r/2584/#comment17182>

    If you are treating status as a boolean then "status" as a name doesn't really work. "reachable" would be better.



/team/group/pimp_my_sip/res/res_sip/sip_options.c
<https://reviewboard.asterisk.org/r/2584/#comment17181>

    This doesn't need to be here.


- Joshua Colp


On May 31, 2013, 8:05 p.m., Kevin Harwell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2584/
> -----------------------------------------------------------
> 
> (Updated May 31, 2013, 8:05 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-21499
>     https://issues.asterisk.org/jira/browse/ASTERISK-21499
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Completed the work that had been started for SIP qualify support in the new channel driver.  Qualify requests are now sent for every contact associated with an endpoint.  When a contact is registered a check is done to make sure it is available and the round trip time is stored for reference.  The contact continues to be checked at a specified interval (qualify_frequency - now set on the AOR) until it is removed.
> 
> A CLI command ("sip qualify <endpoint>") is also available that when issued will send qualifies for all contacts on the given endpoint.  Also added CLI command "sip show endpoint <endpoint>" that will show all the contacts and their status for the given endpoint.
> 
> 
> Diffs
> -----
> 
>   /team/group/pimp_my_sip/include/asterisk/res_sip.h 390336 
>   /team/group/pimp_my_sip/res/res_sip.c 390336 
>   /team/group/pimp_my_sip/res/res_sip/location.c 390336 
>   /team/group/pimp_my_sip/res/res_sip/sip_configuration.c 390336 
>   /team/group/pimp_my_sip/res/res_sip/sip_options.c 390336 
>   /team/group/pimp_my_sip/res/res_sip_registrar.c 390336 
> 
> Diff: https://reviewboard.asterisk.org/r/2584/diff/
> 
> 
> Testing
> -------
> 
> Manually tested by registering contacts and making sure the qualify requests were sent out and scheduled.
> 
> 
> Thanks,
> 
> Kevin Harwell
> 
>

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