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

[Mark Michelson]

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



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

    Instead of documenting the valid values this way, I suggest using an <enumlist> element. This way, it's easier to tack on new values in case we add things like "LAGGED" like chan_sip has.



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

    Optimization suggestion: Since the only way you will ever return CMP_MATCH is if arg is non-NULL, you can short-circuit very early in this function and return 0 if arg is NULL.
    
    Though admittedly, it seems close to impossible for the arg to ever actually be NULL here.



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

    How does this case happen? I don't see any place that sets the data's contact to NULL.



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

    There is a red blob here.



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

    The logic here is screwy. Since one of the flags for ao2_find() is OBJ_NODATA, it means that ao2_find() will return NULL every time. So it appears that this will never actually delete the scheduler entry since the if (!data) will always evaluate true.



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

    An issue I have with this CLI command is that, aside from the case where an endpoint cannot be retrieved, there is no visual feedback given. Messages such as the following would be good:
    
    "Endpoint <blah> has no AoRs configured"
    "Sending qualify to endpoint <foo> contact <bar>"
    
    That sort of thing.



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

    With CLI commands, your best bet when reporting problems is using ast_cli() either instead of ast_log(). There's no guarantee that log messages are going to the CLI, but ast_cli() will always print there.



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

    A CLI command to show individual endpoints is useful, but this seems like an odd file for it to be in. Perhaps it should exist alongside the "sip show endpoints" command in sip_configuration.c?


- Mark Michelson


On June 11, 2013, 3:43 p.m., Kevin Harwell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2584/
> -----------------------------------------------------------
> 
> (Updated June 11, 2013, 3:43 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 391402 
>   /team/group/pimp_my_sip/res/res_sip.c 391402 
>   /team/group/pimp_my_sip/res/res_sip/location.c 391402 
>   /team/group/pimp_my_sip/res/res_sip/sip_configuration.c 391402 
>   /team/group/pimp_my_sip/res/res_sip/sip_options.c 391402 
>   /team/group/pimp_my_sip/res/res_sip_registrar.c 391402 
> 
> 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/20130613/95c0c7e8/attachment-0001.htm>