[asterisk-dev] [Code Review] 3050: PJSIP: Add Path header support

[Matt Jordan]

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



/trunk/res/res_pjsip.c
<https://reviewboard.asterisk.org/r/3050/#comment19778>

    This could use some doxygen, particularly with how token is used



/trunk/res/res_pjsip.c
<https://reviewboard.asterisk.org/r/3050/#comment19777>

    Allocation of req_data can still fail here. You need to check for allocation failure before using the object.
    
    Really, RAII_VAR isn't terribly useful here. You have a single off nominal return path, and since you have to manipulate the reference count prior to pjsip_sip_endpt_send_request, you have to decrement the reference there anyway.



/trunk/res/res_pjsip_path.c
<https://reviewboard.asterisk.org/r/3050/#comment19780>

    There's a number of assumptions being made here about the allowed size of users/domains that I'm not sure are guaranteed to always be true. The same is also true for the size of the two concatanated together - in fact, AST_UUID_STR_LEN is a poor choice here, as it is defined to be substantially smaller than both:
    
    #define AST_UUID_STR_LEN (36 + 1)
    
    I'd suggest duplicating the username/domain on the stack, calculating the length of both, then creating the ID from the actual length of the user/domain.



/trunk/res/res_pjsip_path.c
<https://reviewboard.asterisk.org/r/3050/#comment19781>

    Restructure this differently - if you have an alias, you don't need to create the ID the first time.
    
    I'd also get rid of the RAII_VAR, since it is only needed if a domain_alias has been defined.
    
    if ((alias = ast_sorcery_by_id(...)) {
      // construct ID with alias
      ao2_ref(alias, -1);
    } else {
      // construct ID normal way
    }



/trunk/res/res_pjsip_path.c
<https://reviewboard.asterisk.org/r/3050/#comment19779>

    Blob. There's a few other in this file; you may want to do a whitespace cleanup



/trunk/res/res_pjsip_path.c
<https://reviewboard.asterisk.org/r/3050/#comment19782>

    Because this is using an ao2_callback to get the contact, it's going to be rather expensive. If we were searching based on the contact's typical hash key this wouldn't be a problem - but here, this is an O(n) search.
    
    I'm not sure how we typically use the contacts on an AOR, but if we're iterating over the container a lot, an ao2 container may not be the best choice of storage. ao2 containers with linear searches perform much worse than the linkedlist macros.
    
    Also: why OBJ_UNLINK? Why would you want to remove the contact from the AOR when all you're trying to get is the Path header?



/trunk/res/res_pjsip_path.c
<https://reviewboard.asterisk.org/r/3050/#comment19783>

    I'd imagine that technically, any success code is allowed. This should probably be >= 200 && < 300.
    
    You could also reduce indentation by inverting the logic on this statement and bailing early if this is not a REGISTER_METHOD or not successful.


- Matt Jordan


On Dec. 5, 2013, 7:06 p.m., opticron wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3050/
> -----------------------------------------------------------
> 
> (Updated Dec. 5, 2013, 7:06 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-21084
>     https://issues.asterisk.org/jira/browse/ASTERISK-21084
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This adds Path support to chan_pjsip in res_pjsip_path.c with minimal additions in res_pjsip_registrar.c to store the path and additions in res_pjsip_outbound_registration.c to enable advertisement of path support to registrars and intervening proxies.
> 
> Path information is stored on contacts and is enabled via Address of Record (AoRs) and Registration configuration sections.
> 
> While adding path support, it became necessary to be able to add SIP supplements that handled messages outside of sessions, so a framework for handling these types of hooks was added in parallel to the already-existing session supplements and several senders of out-of-dialog requests were refactored as a result.
> 
> 
> Diffs
> -----
> 
>   /trunk/res/res_pjsip_t38.c 403395 
>   /trunk/res/res_pjsip_registrar.c 403395 
>   /trunk/res/res_pjsip_refer.c 403395 
>   /trunk/res/res_pjsip_path.c PRE-CREATION 
>   /trunk/res/res_pjsip_outbound_registration.c 403395 
>   /trunk/res/res_pjsip_notify.c 403395 
>   /trunk/res/res_pjsip_mwi.c 403395 
>   /trunk/res/res_pjsip_messaging.c 403395 
>   /trunk/res/res_pjsip_header_funcs.c 403395 
>   /trunk/res/res_pjsip_diversion.c 403395 
>   /trunk/res/res_pjsip_caller_id.c 403395 
>   /trunk/res/res_pjsip/pjsip_options.c 403395 
>   /trunk/res/res_pjsip/pjsip_distributor.c 403395 
>   /trunk/res/res_pjsip/location.c 403395 
>   /trunk/res/res_pjsip.c 403395 
>   /trunk/include/asterisk/res_pjsip_session.h 403395 
>   /trunk/include/asterisk/res_pjsip.h 403395 
>   /trunk/channels/chan_pjsip.c 403395 
> 
> Diff: https://reviewboard.asterisk.org/r/3050/diff/
> 
> 
> Testing
> -------
> 
> This passes the tests covered by the review at https://reviewboard.asterisk.org/r/3051/
> 
> These tests were converted to work with chan_pjsip from their chan_sip counterparts.
> 
> 
> Thanks,
> 
> opticron
> 
>

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