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

Fri Mar 12 06:14:42 CST 2010

> On 2010-03-11 14:09:12, Mark Michelson wrote:
> > 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!

Thank you very much for all your comments

Are there any formatting tools that you would recommend for whitespace, tabbing etc?

> On 2010-03-11 14:09:12, Mark Michelson wrote:
> > trunk/channels/sip/reqresp_parser.c, lines 162-205
> > <https://reviewboard.asterisk.org/r/549/diff/1/?file=8528#file8528line162>
> >
> >     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.

Thanks for identifying this issue. I had not realised that "lr" was a special case. I will try again

> On 2010-03-11 14:09:12, Mark Michelson wrote:
> > trunk/channels/sip/include/reqresp_parser.h, line 54
> > <https://reviewboard.asterisk.org/r/549/diff/1/?file=8526#file8526line54>
> >
> >     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.
> 
> Mark Michelson wrote:
>     In case the sheer number of parameters wasn't enough of a reason for you to make this change, there are also some other reasons, mostly having to do with the fact that all output pointers can be specified NULL. There are several cases where you can end up with less-than-desirable results.
>     
>     For instance let's take the following URI into account:
>     
>     sip:user at host:5060;param1=value1?header1=value2
>     
>     Let's look at a particular combination that has some unexpected results:
>     If you call parse_uri_full with headers == NULL and uriparams == NULL, but remainder (or residue as it's called in the function definition) non-NULL, then on returning, residue will be set to "host" and port will be set to "5060;param1=value1?header1=value2"
>     
>     If you change the function prototype to take a struct into which all the output parameters are placed, then there's no worry about the various combinations of NULL and non-NULL pointers to have to deal with.

I think you have described a bug with residue="host". Would my change at line 133 of the .c fix this?

I was copying two aspects of the original parse_uri function here. The first was the separate variables and the second was the ability to pass NULL if one did not want to terminate the string at the detected uri component. The former made it a bit easier to reimplement the parse_uri function and, although I have no idea why one would want to do it, the latter is retained for backward compatibility.

If the requirement for passing NULL **char pointers can be dropped then I will change to a struct

> On 2010-03-11 14:09:12, Mark Michelson wrote:
> > trunk/channels/sip/reqresp_parser.c, lines 211-213
> > <https://reviewboard.asterisk.org/r/549/diff/1/?file=8528#file8528line211>
> >
> >     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?

Yes this is on purpose. Many message-headers contain their own parameters after the uri which need to be returned for further parsing. If the uri is in addr-spec format then the divide between uri parameters and message-header parameters can only be determined after uri parsing e.g. 

To: user at host;param1=apple;transport=tcp;tag=1234

hence the need for the residue. param1 however is unambiguously not a message-header parameter but an unsupported uri parameter   

> On 2010-03-11 14:09:12, Mark Michelson wrote:
> > trunk/channels/sip/reqresp_parser.c, line 545
> > <https://reviewboard.asterisk.org/r/549/diff/1/?file=8528#file8528line545>
> >
> >     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."

To be honest these were all just debug outputs that I had in while getting the tests to pass. I intended to remove them before submission but if they are useful I will leave them in but in a friendlier format such as "pass=%s (expected %s)\n"

> On 2010-03-11 14:09:12, Mark Michelson wrote:
> > trunk/channels/sip/reqresp_parser.c, lines 918-920
> > <https://reviewboard.asterisk.org/r/549/diff/1/?file=8528#file8528line918>
> >
> >     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.

I will check this but I think I needed first_bracket incremented irrespective of the if statement

> On 2010-03-11 14:09:12, Mark Michelson wrote:
> > trunk/channels/sip/reqresp_parser.c, lines 932-934
> > <https://reviewboard.asterisk.org/r/549/diff/1/?file=8528#file8528line932>
> >
> >     Can you give an example where this happens?

I was/am looking at whether the display-name can be identified in-place rather than copied into another part of memory as done currently by get_calleridname(). In so doing I found that the trailing quote of a quoted display-name and the trailing space of a token display-name could be used for the string terminator. A common error though is omitting the trailing space in a token display name e.g. 
Nick Lewis<1234 at myself.com>
Rather than reject this it would be better if the display-name were terminated at the bracket. 

> On 2010-03-11 14:09:12, Mark Michelson wrote:
> > trunk/channels/sip/reqresp_parser.c, line 1090
> > <https://reviewboard.asterisk.org/r/549/diff/1/?file=8528#file8528line1090>
> >
> >     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.

Ok I will use linkedlists.h Thanks for pointing it out

> On 2010-03-11 14:09:12, Mark Michelson wrote:
> > trunk/channels/sip/reqresp_parser.c, line 221
> > <https://reviewboard.asterisk.org/r/549/diff/1/?file=8528#file8528line221>
> >
> >     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.

Can you please help me a little more to understand comments 2&3 
- scrub that, it is clear below

(I think 4 is consistent with other unit tests in this file) 

> On 2010-03-11 14:09:12, Mark Michelson wrote:
> > trunk/channels/sip/reqresp_parser.c, lines 1217-1227
> > <https://reviewboard.asterisk.org/r/549/diff/1/?file=8528#file8528line1217>
> >
> >     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.

The code was lifted from pre-existing unit tests in this file but I agree that it would be far more useful with specific test fail messages so will change 

- Nick

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

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