[asterisk-dev] [Code Review] 2471: Basic res_sip XML documentation

Sun May 19 02:08:05 CDT 2013

> On May 13, 2013, 2:43 p.m., Mark Michelson wrote:
> > trunk/res/res_sip.c, lines 66-67
> > <https://reviewboard.asterisk.org/r/2471/diff/3/?file=37633#file37633line66>
> >
> >     This first sentence is a bit weird-sounding. Try rewording it.

I've removed this info completely, as I probably am not correct

> On May 13, 2013, 2:43 p.m., Mark Michelson wrote:
> > trunk/res/res_sip.c, line 69
> > <https://reviewboard.asterisk.org/r/2471/diff/3/?file=37633#file37633line69>
> >
> >     Don't abbreviate words like "register" in documentation.

my bad shorthand while trying to get this done. fixed

> On May 13, 2013, 2:43 p.m., Mark Michelson wrote:
> > trunk/res/res_sip.c, lines 145-147
> > <https://reviewboard.asterisk.org/r/2471/diff/3/?file=37633#file37633line145>
> >
> >     It's probably best not to list both "invite" and "reinvite" as options here since people will assume there's a difference between the two. In actuality, "reinvite" is the better option name, but we'll accept "invite" as well since people are used to that from chan_sip and because even though it is a reinvite, the method name is still "INVITE"

Tweaked text, could also make the 'default' reinvite if we alter the sorcery register in sip_configuration.

> On May 13, 2013, 2:43 p.m., Mark Michelson wrote:
> > trunk/res/res_sip.c, lines 241-251
> > <https://reviewboard.asterisk.org/r/2471/diff/3/?file=37633#file37633line241>
> >
> >     You should explain what the different options mean. People moving from chan_sip's configuration could be unfamiliar with the differences in the terms.

I'm not that familiar myself, left as is for now

> On May 13, 2013, 2:43 p.m., Mark Michelson wrote:
> > trunk/res/res_sip.c, line 360
> > <https://reviewboard.asterisk.org/r/2471/diff/3/?file=37633#file37633line360>
> >
> >     s/Asyncronous/Asynchronous/
> >     
> >     (Actually, this is one of those cases where I dread correcting the spelling because I have no idea if this spelling is considered proper for en_AU)

no i just have no spell check on vi..

> On May 13, 2013, 2:43 p.m., Mark Michelson wrote:
> > trunk/res/res_sip.c, lines 381-383
> > <https://reviewboard.asterisk.org/r/2471/diff/3/?file=37633#file37633line381>
> >
> >     Hm, there's an external media address option for endpoints, too. If an endpoint has this option set and a transport has this set, which is used?
> >     
> >     I believe it would be the endpoint.

I'm confused.. didn't change

> On May 13, 2013, 2:43 p.m., Mark Michelson wrote:
> > trunk/res/res_sip.c, lines 457-463
> > <https://reviewboard.asterisk.org/r/2471/diff/3/?file=37633#file37633line457>
> >
> >     This description is both right and wrong. Configuring an AoR section does allow you to group multiple contacts together. It also does allow for you to dial multiple contacts by using PJSIP_DIAL_CONTACTS.
> >     
> >     However, that's not its only purpose. Aside from specifying a URI in a dialstring, this is the only way possible to allow for an endpoint to be contacted. So while it can be used for grouping multiple contacts together, specifying an AoR is the one way that exists to make an endpoint reachable by Asterisk.

made an improvement think it reads a bit better.

> On May 13, 2013, 2:43 p.m., Mark Michelson wrote:
> > trunk/res/res_sip.c, lines 472-477
> > <https://reviewboard.asterisk.org/r/2471/diff/3/?file=37633#file37633line472>
> >
> >     "Default keep alive time for an AoR" is a bit ... off.
> >     
> >     First off, the setting applies to contacts bound dynamically to the AoR via REGISTER requests rather than to the AoR itself.
> >     
> >     "Keep alive" makes it sound like we will do something at the interval in order to maintain the contact. In actuality, this is the expiration time for the contact.

Made slight change still probably not right.

> On May 13, 2013, 2:43 p.m., Mark Michelson wrote:
> > trunk/res/res_sip_acl.c, lines 51-56
> > <https://reviewboard.asterisk.org/r/2471/diff/3/?file=37634#file37634line51>
> >
> >     These should make reference to the acl.conf file since that is where the named ACLs can be located.

might still not understood what you meant here.

> On May 13, 2013, 2:43 p.m., Mark Michelson wrote:
> > trunk/res/res_sip_outbound_registration.c, lines 42-43
> > <https://reviewboard.asterisk.org/r/2471/diff/3/?file=37636#file37636line42>
> >
> >     This sounds a bit strange since this doesn't define SIP registration procedures or operations using PJProject to achieve that :)
> >     
> >     This actually just specifies configuration options available.

Dropped mention to PJProject

> On May 13, 2013, 2:43 p.m., Mark Michelson wrote:
> > trunk/res/res_sip_outbound_registration.c, lines 62-66
> > <https://reviewboard.asterisk.org/r/2471/diff/3/?file=37636#file37636line62>
> >
> >     The description and synopsis are basically the same thing here.

Didn't want to have the 'time' part in the synopsis, and there isn't much 'explanation' we can put is there?

- Brad

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

On May 13, 2013, 7:54 a.m., Brad Latus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2471/
> -----------------------------------------------------------
> 
> (Updated May 13, 2013, 7:54 a.m.)
> 
> 
> Review request for Asterisk Developers and Matt Jordan.
> 
> 
> Bugs: ASTERISK-21668
>     https://issues.asterisk.org/jira/browse/ASTERISK-21668
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Start of XML documentation for res_sip, not complete but a start.
> 
> 
> Diffs
> -----
> 
>   trunk/res/res_sip.c 387348 
>   trunk/res/res_sip_acl.c 387348 
>   trunk/res/res_sip_endpoint_identifier_ip.c 387348 
>   trunk/res/res_sip_outbound_registration.c 387348 
> 
> Diff: https://reviewboard.asterisk.org/r/2471/diff/
> 
> 
> Testing
> -------
> 
> Ran xmllint - no errors.
> No warnings about missing XML during asterisk startup.
> 
> 
> Thanks,
> 
> Brad Latus
> 
>

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