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

Mon May 13 09:43:57 CDT 2013

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


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16662>

    missing the word "more" after "one or"
    
    Also, since this is the first usage of "AoR" consider expanding to its full meaning this first time just so it's 100% clear what is meant.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16663>

    Be sure in documentation to always use the properly capitalized "PJSIP"
    
    Also, your first use of "AoR" was capitalized like so, so keep it consistent.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16664>

    Formulate this as a proper SIP URI:
    
    sip:5000@[11::33]


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16665>

    s/tgrab/grab/
    
    Actually, change the word "grab" to "use" here.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16667>

    This first sentence is a bit weird-sounding. Try rewording it.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16670>

    In documentation, application names are typically capitalized.
    
    Dial(PJSIP/local-ep/local-aor/mycontact)


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16671>

    Don't abbreviate words like "register" in documentation.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16672>

    This could use a bit more explanation.
    
    aggregate_mwi, when enabled condenses message waiting notifications from multiple mailboxes into a single NOTIFY. If disabled, then individual NOTIFYs corresponding to each mailbox are sent.
    
    Default value is "yes"


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16673>

    Two things:
    
    1) The description focuses too much on what happens if an auth is *not* specified as opposed to what the option is. Indicate that this is the name of an auth section in the configuration
    
    2) Emphasize that this option applies only to inbound authentication. In other words, if an endpoint sends a request to us, and there is an auth option configured, then we will attempt to challenge the request for authentication


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16674>

    The quotation marks around "Name" aren't required for caller ID. Also, expand "Num" to "Number"


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16675>

    This could use further explanation. Since this option is a bit complicated to explain in text, your best bet would be to give a small explanation of each option and then to point to the wiki page here: https://wiki.asterisk.org/wiki/display/AST/SIP+Direct+Media+Reinvite+Glare+Avoidance
    
    Right now, that page focuses more on the option available in sip.conf in Asterisk 11, but the intention is the same. A section can be added for the Asterisk 12 res_sip.conf equivalent.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16677>

    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"


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16679>

    Wording the documentation like a question isn't a good idea. Make a statement of what the option does instead:
    
    "Determines whether direct media communication is allowed"


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16680>

    Probably should add the word "media" before NAT session just so it's clear that we're referring to NAT of the media as opposed to signaling.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16681>

    s/superceeds/supercedes/


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16682>

    You should explain what the different options mean. People moving from chan_sip's configuration could be unfamiliar with the differences in the terms.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16683>

    Remove the word "it"


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16684>

    These could use more explanation.
    
    trust_id_inbound: This determines whether we accept identification from the endpoint that is received in a P-Asserted-Identity or Remote-Party-ID header. If set false, then we will always use the configured callerid from res_sip.conf as the identity for the endpoint.
    
    trust_id_outbound: This determines whether we will send identification information to the endpoint that has been marked as private. If set false, then private caller ID will not be forwarded to the endpoint.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16685>

    s/Endpoints/Endpoint's


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16686>

    s/Authorization/Authentication/
    
    This holds true throughout the entire section.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16687>

    These could use a bit of clarification. No matter which is used, we are going to send an MD5 hash over the line. What this determines is how the authentication is stored in configuration. So if they have set to "userpass" then we will read a username and password from configuration and plug that into the appropriate places to generate an MD5 hash. If "md5" is specified, then we take the hash they have specified as-is.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16691>

    Note that this option is only used when auth_type is set to "md5"


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16690>

    Note that this option is only used when auth_type is set to "userpass"


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16688>

    Put a <literal> tag around the word "auth" here.
    
    The same comment applies to all "type" option names.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16692>

    May as well just remove this :)


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16696>

    It's also used during when an endpoint registers when checking the domain of the AoR to which the endpoint is binding.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16699>

    We're about to have websocket support as well. Might as well go ahead and add it as the third in the list.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16700>

    This seems out of place?


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16702>

    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)


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16705>

    IP address and optional port.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16710>

    The "TCP ONLY" here is a bit misleading because the option is actually only valid when you specify protocol=tls. If you specify protocol=tcp, then this option is not used.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16707>

    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.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16711>

    Another case where "TCP ONLY" is misleading.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16708>

    Specify that the format for this is an IP address and network mask.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16719>

    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.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16715>

    My spellchecker prefers "dialing" over "dialling"


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16717>

    You have a red blob here.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16720>

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


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16721>

    This could use a bit of explanation considering that there is also a mailbox option for endpoints.
    
    This option applies specifically when an external entity subscribes for message waiting indications. When they subscribe to the AoR, it results in a subscription being made for the mailboxes specified by this option.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16722>

    s/explict/explicit/


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16723>

    s/using an/that can bind to an/


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16724>

    s/Maximium/Maximum/


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16725>

    I believe rather than using <literal> around "remove_existing" the proper tag to use is <replaceable>
    
    Someone might correct me on that though.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16726>

    s/Minium/Minimum


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16728>

    Both of these sentences are questions that end with periods. The first should be changed to a statement of what the option does. The second can remain as a question. Just change the punctuation.


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16727>

    s/receving/receiving/


trunk/res/res_sip.c
<https://reviewboard.asterisk.org/r/2471/#comment16729>

    See previous comment about <literal> tags being used with option names.


trunk/res/res_sip_acl.c
<https://reviewboard.asterisk.org/r/2471/#comment16731>

    s/ways main ways/main ways/


trunk/res/res_sip_acl.c
<https://reviewboard.asterisk.org/r/2471/#comment16730>

    s/possible create/possible to create/


trunk/res/res_sip_acl.c
<https://reviewboard.asterisk.org/r/2471/#comment16732>

    These should make reference to the acl.conf file since that is where the named ACLs can be located.


trunk/res/res_sip_endpoint_identifier_ip.c
<https://reviewboard.asterisk.org/r/2471/#comment16733>

    Wording is redundant. Try "Module that identifies endpoints via source IP address"


trunk/res/res_sip_outbound_registration.c
<https://reviewboard.asterisk.org/r/2471/#comment16734>

    Maybe it's just me, but I dislike when terms like "SIP trunks" are used since it sounds too much like the PSTN.


trunk/res/res_sip_outbound_registration.c
<https://reviewboard.asterisk.org/r/2471/#comment16735>

    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.


trunk/res/res_sip_outbound_registration.c
<https://reviewboard.asterisk.org/r/2471/#comment16736>

    s/separate the/separate from the/


trunk/res/res_sip_outbound_registration.c
<https://reviewboard.asterisk.org/r/2471/#comment16737>

    s/would consist of/consists of/


trunk/res/res_sip_outbound_registration.c
<https://reviewboard.asterisk.org/r/2471/#comment16738>

    The wording of the synopsis is a bit strange. Consider:
    
    "Determines whether authentication rejections are deemed as permanent failures. If permanent, then registration is not attempted again after an authentication rejection until configuration is reloaded"


trunk/res/res_sip_outbound_registration.c
<https://reviewboard.asterisk.org/r/2471/#comment16739>

    The description and synopsis are basically the same thing here.


trunk/res/res_sip_outbound_registration.c
<https://reviewboard.asterisk.org/r/2471/#comment16740>

    I think the synopsis here is the template you were using for all options before filling them in with more details.


trunk/res/res_sip_outbound_registration.c
<https://reviewboard.asterisk.org/r/2471/#comment16741>

    You can remove the first "Retry" from this sentence.


trunk/res/res_sip_outbound_registration.c
<https://reviewboard.asterisk.org/r/2471/#comment16742>

    Put a space after the period.


trunk/res/res_sip_outbound_registration.c
<https://reviewboard.asterisk.org/r/2471/#comment16743>

    s/Explict/Explicit/


trunk/res/res_sip_outbound_registration.c
<https://reviewboard.asterisk.org/r/2471/#comment16744>

    Actually, this option is the name of a transport that has been specified in res_sip.conf


- Mark Michelson


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/20130513/1d6f1399/attachment-0001.htm>