[asterisk-dev] [Code Review] Introduce function for parsing ABNF structure {name-andor-addr = name-addr / addr-spec} in sip messages

Thu Mar 11 14:09:12 CST 2010

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

This is my first pass through this review request. Even though there are many areas that need improvement, I have to say that this is an excellent addition to the code, and you get bonus points from me for including unit tests along with the code changes.

In addition to what I marked below, please also make an effort when posting your next diff to remove as much trailing whitespace as you can from the diff so that there aren't as many red blotches on the review.

Thanks!

trunk/channels/sip/include/reqresp_parser.h
<https://reviewboard.asterisk.org/r/549/#comment3678>

    I have a suggestion for this function.

    Seeing the unruly number of parameters to this function, I think a better approach would be to wrap all the output parameters in a struct instead of having each individual one returned.

    Also, "remainder" is spelled incorrectly here.

trunk/channels/sip/include/reqresp_parser.h
<https://reviewboard.asterisk.org/r/549/#comment3677>

    I have the same suggestions for this function as for parse_uri_full()

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3697>

    These two lines need to be unindented by one tab.

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3698>

    This line needs to be unindented by one tab.

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3699>

    Place braces around single line if statements.

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3700>

    Indent this one tab.

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3701>

    Spaces around assignment operator.

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3702>

    Spaces around assignment operator.

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3708>

    It is somewhat common for certain parameters to not contain an "=" sign, particularly the "lr" parameter. If I have read this loop correctly, we will

    1) Not properly parse any parameter that does not contain an "="
    2) Not parse any further parameters if we come across one that does not contain an "="

    I think this loop needs to be rewritten in a manner that does not depend on an "=" being present for all parameters.

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3703>

    spaces around assignment operators.

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3704>

    spaces around assignment opersators.

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3707>

    Spaces around asssigment operators and spaces after commas.

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3705>

    These two lines each need to be indented one more tab.

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3709>

    Based on sip_parse_uri_fully_test, this may actually be intended behavior, but I'll still remark on this anyway.

    If you have a uri like:

    name at host;param1=apple;transport=tcp;param2=banana

    Then the transport value will be stored in the uriparams struct, and "param2=banana" will be stored in residue. However, "param1=apple" is just lost in this case.

    If this is being done on purpose, can you add a comment somewhere, preferably to the doxygen for this function why this is being done this way?

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3694>

    So, I made all my comments on your other unit test first, but most of them apply here, especially with regards to

    1) Indentation inconsistency
    2) Use of an open coded linked list
    3) Odd structure of the while loop
    4) Use of a giant compound if statement instead of many small ones.

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3687>

    I like this update; however, I it needs to be structured the same as the edit you made further up in the function. In other words, instead of having each variable output with slashes separating them, it is better to print them like "name = blah, pass = blah, etc."

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3688>

    This change makes no sense to me.

    Edit: Okay, after looking further in the function, I see what you are doing here, although I personally like the way it was better.

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3691>

    Can you give an example where this happens?

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3690>

    This line needs to be unindented one tab.

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3692>

    Lines 916 and 918 are each indented one tab too many.

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3686>

    Spaces after commas.

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3685>

    Indent this line one tab further

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3683>

    Spaces around assignment operator.

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3693>

    Spaces after commas.

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3684>

    spaces around assignment operator.

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3682>

    This test in general has issues regarding the use of spaces and tabs. All lines must be indented with tabs in Asterisk. The code here seems to switch seemingly randomly between the use of one or the other.

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3679>

    Please don't use open coded linked lists like this. It is much easier for everyone involved if you use the linked list macros defined in linkedlists.h

    Alternatively, if you don't want to use those, you could easily get by with the use of an array here instead.

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3681>

    One way to make things a bit less "wordy" in this test would be to initialize all fields of the testdata at once. To illustrate, you can do something like this:

    struct testdata testdata1 = {
        .desc = "blah",
        .uri = "blah",
        ...
    };

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3680>

    This loop is set up quite strangely. Why have an infinite loop with only one breaking possibility? This, to me, is much more indicative of what the loop's purpose is:

    while(testdataptr) {
        <body of loop>
        testdataptr = testdataptr->next;
    }

    Of course, if you change to using the macros in linkedlists.h, then this loop will simply change to AST_LIST_TRAVERSE instead.

trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/549/#comment3696>

    It would be better if each of the conditions between ||s could be in its own if statement. That way, you can use ast_test_status_update to more specifically tell which part of the parsing failed.

- Mark

On 2010-03-11 03:12:54, Nick Lewis wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/549/
> -----------------------------------------------------------
> 
> (Updated 2010-03-11 03:12:54)
> 
> 
> Review request for Asterisk Developers and David Vossel.
> 
> 
> Summary
> -------
> 
> Many sip headers in many sip methods contain the ABNF structure
>  name-andor-addr = name-addr / addr-spec
> Examples include the to-header, from-header, contact-header, replyto-header, referto-header, referredby-header, and passertedid-header
> 
> At the moment chan_sip.c makes various different attempts to parse this name-andor-addr structure for each header type and for each sip method with sometimes limited degrees of success.
> 
> I here introduce a dedicated function for parsing the name-andor-addr ABNF structure that can be used irrespective of the specific method or header that contains the structure
> 
> (The function is also suited to parsing the name-addr ABNF structure without the addr-spec option. Examples in chan_sip.c include the recordroute-header, route-header, remotepartyid-header and diversion-header.)
> 
> 
> This addresses bug 16708.
>     https://issues.asterisk.org/view.php?id=16708
> 
> 
> Diffs
> -----
> 
>   trunk/channels/sip/include/reqresp_parser.h 249060 
>   trunk/channels/sip/include/sip.h 249060 
>   trunk/channels/sip/reqresp_parser.c 249060 
> 
> Diff: https://reviewboard.asterisk.org/r/549/diff
> 
> 
> Testing
> -------
> 
> Tested using the corresponding unit test function
> 
> 
> Thanks,
> 
> Nick
> 
>