[asterisk-dev] [Code Review] 3817: res_pjsip_notify: Add the ability for PJSIPNotify AMI command and pjsip send notify CLI command to send to a URI instead of an endpoint

[Mark Michelson]

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



/trunk/include/asterisk/res_pjsip.h
<https://reviewboard.asterisk.org/r/3817/#comment23063>

    This function isn't necessary. When PJSIP is passed a URI string, PJSIP will perform URI validation for us and return an error if a badly-formed URI is passed in.



/trunk/res/res_pjsip_notify.c
<https://reviewboard.asterisk.org/r/3817/#comment23062>

    This second line is indented 7 tabs. That's just a touch overkill :)



/trunk/res/res_pjsip_notify.c
<https://reviewboard.asterisk.org/r/3817/#comment23064>

    You really need to have an endpoint present in the calls to ast_sip_create_request() and ast_sip_send_request(). The biggest reason is that if the NOTIFY is responded to with a 401, we won't be able to authenticate since we have no endpoint configuration to use.
    
    The way to do this is to use ast_sip_default_outbound_endpoint() to get the appropriate endpoint. If it returns NULL, then the system is not configured to be able to send requests to arbitrary URIs.
    
    If you want to test this out yourself, then you'll need to create an endpoint in pjsip.conf and then in the type=global section, add "default_outbound_endpoint=<endpoint_name>".



/trunk/res/res_pjsip_notify.c
<https://reviewboard.asterisk.org/r/3817/#comment23065>

    The way this is set up, if you were to get on the CLI and type "pjsip notify foo ur<tab>" then the generation function will print the word "uri" twice, once for state == 0 and once for state == 1.
    
    To properly account for this, you need to break this up into the case where word is zero-length and where word is not zero-length.
    
    if (ast_strlen_zero(word)) {
        if (state == 0) {
            c = ast_strdup("endpoint");
        } else if (state == 1) {
            c = ast_strdup("uri");
        }
    } else if (state == 0) {
        if (strncasecmp(word, "endpoint", wordlen) {
            c = ast_strdup("endpoint");
        } else if (strncasecmp(word, "uri", wordlen) {
            c = ast_strdup("uri");
        }
    }
    return c;



/trunk/res/res_pjsip_notify.c
<https://reviewboard.asterisk.org/r/3817/#comment23061>

    The error message should indicate that an endpoint name or URI is acceptable.


- Mark Michelson


On July 17, 2014, 7:27 p.m., Jonathan Rose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3817/
> -----------------------------------------------------------
> 
> (Updated July 17, 2014, 7:27 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Example:
> 
> Action: PJSIPNotify
> URI: sip:10.10.10.10
> 
> pjsip send notify uri <type> uri <uri> [uri...]
> 
> 
> Adds the ability to use URIs with the PJSIP notify commands instead of endpoints. These mostly work the same as the endpoint notifications.
> 
> 
> Diffs
> -----
> 
>   /trunk/res/res_pjsip_notify.c 418783 
>   /trunk/res/res_pjsip.c 418783 
>   /trunk/include/asterisk/res_pjsip.h 418783 
> 
> Diff: https://reviewboard.asterisk.org/r/3817/diff/
> 
> 
> Testing
> -------
> 
> Used existing endpoint notification and compared it to similar notifies done against URIs.  The outgoing messages looked the same as far as the details I was concerned with went.
> 
> 
> Thanks,
> 
> Jonathan Rose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140718/93f3a0f8/attachment-0001.html>