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

[Nick Lewis]

> On 2010-06-16 12:09:20, David Vossel wrote:
> > 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.
> > 
> >

I will add the missing peer options to sip.conf.sample and work on making the description simpler. I would ideally like to make the default callbackexten the same as the peername so that registration is as simple as setting register=yes for 99% of applications


> On 2010-06-16 12:09:20, David Vossel wrote:
> > trunk/channels/sip/include/sip.h, lines 107-109
> > <https://reviewboard.asterisk.org/r/718/diff/2/?file=10910#file10910line107>
> >
> >     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.

Yes - perhaps also a callbackexten_is_peername global option if this is wanted instead of DEFAULT_CALLBACKEXTEN


> On 2010-06-16 12:09:20, David Vossel wrote:
> > trunk/configs/sip.conf.sample, lines 553-556
> > <https://reviewboard.asterisk.org/r/718/diff/2/?file=10911#file10911line553>
> >
> >     Why is the registry line format information removed here?

I think I meant only to move it down to beneath the description of registration in the [peer]. I will have another go. 


- Nick


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


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
> 
>