[asterisk-dev] [Code Review] Remove need for registration strings in sip.conf

[David Vossel]

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


First off, Thank you for doing this!  I've been wanting this option for awhile now and just haven't gotten the chance to work on it.  The code looks good.  I had a few minor comments but didn't find anything serious.

My main concern about this feature is making sure we get the documentation right.  Some of the new peer options you've added aren't documented in sip.conf.sample.  The tricky part about this feature is documenting it in a way that doesn't make registrations appear to be even more complicated than they already are.  I'm still not sure how this should be done.

 


trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/718/#comment4718>

    take a look at the default_expiry global variable.



trunk/channels/sip/include/sip.h
<https://reviewboard.asterisk.org/r/718/#comment4717>

    Do we need another define for this value? There is already a default expiry value used in sip_register() if none is provided.
    
    The same goes for DEFAULT_CALLBACKEXTEN. If none is provided "s" is already the default used in sip_parse_register_line()
    
    ... Well now looking at your code that is used to build the registry string, it would make it easier if these defaults already existed so they could be used to build the string the same way every time.  My suggestion here is to keep DEFAULT_CALLBACKEXTEN and use it in sip_parse_register() as the default value if no callbackextension is provided instead of the hardcoded "s".  Then use the value of the global "default_expiry" variable instead of the new DEFAULT_CALLBACKEXPIRY define you added. 



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

    Why is the registry line format information removed here?  


- David


On 2010-06-16 10:51:34, Nick Lewis wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/718/
> -----------------------------------------------------------
> 
> (Updated 2010-06-16 10:51:34)
> 
> 
> Review request for Asterisk Developers and David Vossel.
> 
> 
> Summary
> -------
> 
> This change adds the ability to specify the sip registration expiry and registrar host to the peer configuration items. This permits all registration details that are available to the registration string to be also available to the peer definitions. The users of sip.conf can enable registration by setting register=yes in the peer and do not need to concern themselves with the strange format of the registration strings
> 
> Suggest inclusion in 1.8
> 
> 
> Diffs
> -----
> 
>   trunk/channels/chan_sip.c 268968 
>   trunk/channels/sip/include/sip.h 268968 
>   trunk/configs/sip.conf.sample 268968 
> 
> Diff: https://reviewboard.asterisk.org/r/718/diff
> 
> 
> Testing
> -------
> 
> Compile, run and confirm in cli sip show registry
> 
> 
> Thanks,
> 
> Nick
> 
>