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

[Mark Michelson]

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

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.


- Mark


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