[asterisk-dev] [Code Review] 2858: res_pjsip_header_funcs: New module to create PJSIP_HEADER, PJSIPAddHeader and PJSIPRemoveHeader

Tue Sep 17 14:22:25 CDT 2013

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

Thanks for the submission! Despite the number of comments below, this is a really great first effort. Most of the comments below focus on small matters that can be easily corrected, such as coding guidelines violations. I'll take another pass at this once you fix the stuff mentioned here.

/branches/12/res/res_pjsip_header_funcs.c
<https://reviewboard.asterisk.org/r/2858/#comment18864>

    Heh, feel free to put yourself as the Copyright holder and author. Credit where credit is due :)

/branches/12/res/res_pjsip_header_funcs.c
<https://reviewboard.asterisk.org/r/2858/#comment18865>

    Remove the semicolons from the end of your dialplan examples. Also, instead of having // as your comment marker, use a semicolon, since that is how comments are represented in Asterisk config files.

/branches/12/res/res_pjsip_header_funcs.c
<https://reviewboard.asterisk.org/r/2858/#comment18866>

    When you say "remove all P- headers" does that mean remove all headers beginning with "P-" or remove all headers with the literal value "P-" will be removed? Based on context clues, I'm thinking that since you did not include a colon, this will remove headers beginning with "P-", but it's not made as clear as it could be in the initial description.

/branches/12/res/res_pjsip_header_funcs.c
<https://reviewboard.asterisk.org/r/2858/#comment18867>

    See https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines#CodingGuidelines-Don%27tuseunnecessarytypedef%27s

    In general, typedefs shouldn't be used on structures in Asterisk. Note that PJSIP clearly does not have this rule, so I can understand why you may have thought that Asterisk worked the same way.

/branches/12/res/res_pjsip_header_funcs.c
<https://reviewboard.asterisk.org/r/2858/#comment18870>

    The use of rdata->tp_info.pool here worries me. I think that it would be better to use a pool whose lifetime is the same scope as the ast_sip_session. On the dev-list, I pointed out a way to get to the pjsip_dialog's pool. That would be a better choice here. Either that, or the pjsip_inv_session's pool_active.

/branches/12/res/res_pjsip_header_funcs.c
<https://reviewboard.asterisk.org/r/2858/#comment18868>

    See https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines#CodingGuidelines-Nonestedstatementswithoutbraces

    Always use braces for if/while/do/for statements.

/branches/12/res/res_pjsip_header_funcs.c
<https://reviewboard.asterisk.org/r/2858/#comment18869>

    See https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines#CodingGuidelines-Declarationoffunctionsandvariables

    Don't declare variables mid-block. This one can actually be enforced by the compiler by running Asterisk's configure script with --enable-dev-mode. This way, warnings are treated as errors, so you will be unable to compile the code with these variable declarations here.

    Same comment applies to other places where you have declared variables mid-block.

/branches/12/res/res_pjsip_header_funcs.c
<https://reviewboard.asterisk.org/r/2858/#comment18871>

    Since temp's use is limited to this function, I would suggest stack-allocating it using alloca() instead of using the pool.

    Also, I really dislike the name "temp" for variables, so using something more descriptive would be useful.

/branches/12/res/res_pjsip_header_funcs.c
<https://reviewboard.asterisk.org/r/2858/#comment18872>

    See https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines#CodingGuidelines-Codeformatting

    While it's not explicit there, it does state that spacing should be consistent throughout the codebase. This means that you should put spaces around the '+' sign here.

    Also, it's unclear why you're comparing to headerlen + 2. headerlen is the length of the header name (or partial header name) passed into PJSIP_HEADER. l is the total length of the header. Is the + 2 supposed to account for the : and null byte?

/branches/12/res/res_pjsip_header_funcs.c
<https://reviewboard.asterisk.org/r/2858/#comment18873>

    I believe this is a sequence point violation and may have undefined behavior. Separate this into

    ++p;
    p = ast_strip(p);

/branches/12/res/res_pjsip_header_funcs.c
<https://reviewboard.asterisk.org/r/2858/#comment18874>

    Spaces around + sign.

/branches/12/res/res_pjsip_header_funcs.c
<https://reviewboard.asterisk.org/r/2858/#comment18875>

    Rather than using a pool like this, use

    hname = ast_strdupa(data);

    That will create a stack-allocated copy of data and assign it to hname.

/branches/12/res/res_pjsip_header_funcs.c
<https://reviewboard.asterisk.org/r/2858/#comment18877>

    I'm not so certain that this is a requirement for SIP headers. Looking at the ABNF in RFC 3261, there appears not to be a requirement for all header types that a value is provided.

/branches/12/res/res_pjsip_header_funcs.c
<https://reviewboard.asterisk.org/r/2858/#comment18881>

    You can save yourself some allocations (which, admittedly are cheap when using pj_pool_t) by reformatting this as:

    pj_str_t pj_header_name;
    pj_str_t pj_header_value;

    pj_cstr(&pj_header_name, hname);
    pj_cstr(&pj_header_value, hvalue);

    le->hdr = pjsip_generic_string_hdr_create(pool, hname, hvalue);

/branches/12/res/res_pjsip_header_funcs.c
<https://reviewboard.asterisk.org/r/2858/#comment18879>

    See https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines#CodingGuidelines-GeneralRules first bullet point.

    The indentation of the middle two statements is different from the others in this selection. It appears you may have used four spaces to indent instead of a tab.

/branches/12/res/res_pjsip_header_funcs.c
<https://reviewboard.asterisk.org/r/2858/#comment18882>

    Place spaces around the - signs used on these two lines.

/branches/12/res/res_pjsip_header_funcs.c
<https://reviewboard.asterisk.org/r/2858/#comment18885>

    Instead of checking length and using pj_strnicmp2(), just use pjstricmp()

/branches/12/res/res_pjsip_header_funcs.c
<https://reviewboard.asterisk.org/r/2858/#comment18883>

    A better way of doing this would be to set a destroy callback in header_datastore. That way, you'll get called into when the datastore is destroyed, and you'll be given the datastore->data as the argument in the callback.

/branches/12/res/res_pjsip_header_funcs.c
<https://reviewboard.asterisk.org/r/2858/#comment18884>

    Hm, I'm not so sure that UPDATE is correct here.

- Mark Michelson

On Sept. 16, 2013, 12:34 a.m., George Joseph wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2858/
> -----------------------------------------------------------
> 
> (Updated Sept. 16, 2013, 12:34 a.m.)
> 
> 
> Review request for Asterisk Developers and Mark Michelson.
> 
> 
> Bugs: ASTERISK-22498
>     https://issues.asterisk.org/jira/browse/ASTERISK-22498
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> For PJSIP_HEADER, an incoming supplemental session callback is registered that takes the pjsip_hdrs from the incoming session and stores them in a linked list in the session datastore.  Calls to PJSIP_HEADER traverse over the list and return the nth matching header where 'n' is the 'number' argument to the function.
> 
> For PJSIPAddHeader, the first call creates a datastore and linked list and adds the datastore to the session.  The header is then created as a pjsip_hdr and added to the list.  An outgoing supplemental session callback then traverses the list and adds the headers to the outgoing pjsip_msg.
> 
> For PJSIPRemoveHeader, the list created with PJSIPAddHeader is traversed and all matching entries are removed.  As with SIPRemoveHeader, an empty arguments removes all headers previously added.
> 
> Rather than cloning the incoming pjsip_msg or using pjsip_msg to accumulate the outgoing headers, I used a simple AST_LIST. There was a lot of overhead with the clone functions and some tricky behavior with the pjsip_msg/pjsip_hdr internal lists. Using the AST_LISTs cut down on both instructions and memory.  
> 
> All memory allocation is from the pj_pool attached to the session.
> 
> 
> Diffs
> -----
> 
>   /branches/12/res/res_pjsip_header_funcs.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/2858/diff/
> 
> 
> Testing
> -------
> 
> Tested successfully...
> PJSIP_HEADER return the full value of nth specified header from either the incoming session, or headers previously added to the outgoing session by PJSIPHeader.
> PJSIP_HEADER fails if there was no header specified in the function call.
> PJSIP_HEADER fails if there was no datastore on the incoming session (should never happen).
> PJSIP_HEADER fails if the nth header wasn't found.
> 
> PJSIPAddHeader adds a pjsip_hdr structure to the linked list when the input string is properly formatted as "header_name:\s*header_value".
> PJSIPAddHeader fails if no ':' was found in the input string.
> PJSIPAddHeader fails if after parsing either the header name or value is empty.
> 
> PJSIPRemoveHeader removes all matching headers from the linked list when a partial header name is specified.
> PJSIPRemoveHeader removes all matching headers from the linked list when a full header is specified with a trailing ':'.
> PJSIPRemoveHeader removes all previously added header from the linked list when no header is specified.
> PJSIPRemoveHeader returns successfully (silently) if there was no linked list.
> 
> 
> Thanks,
> 
> George Joseph
> 
>

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