[asterisk-dev] [Code Review] SIP from-header is parsed twice

[Nick Lewis]

> On 2010-06-16 16:11:57, David Vossel wrote:
> > Thanks again for your contributions Nick!  I've looked over the code and like where you are headed with this.  I had a few ideas, let me know what you think.
> > 
> > 1. What if get_source() was more like parse_uri in that it took "char **" as output parameters.  That way we can decouple setting the sip_pvt stringfields from the parsing function making it unit testable.  At that point it would be good to move get_source into reqresp_parser.c.
> > 
> > 2. What if instead of calling the new get_source() function before the functions that require the "From" header information we just detected whether or not we needed to retrieve that information or not within the function it is required in.  I feel a little uneasy about decoupling the parsing from those functions since it would require us keeping track of making sure get_source() was called before them.  From a maintenance standpoint that might be a little rough.
> 
> Nick Lewis wrote:
>     1. I like this componentization. Regarding removing from chan_sip.c what are the principles behind where the boundaries lie. My concern is that all channels are in reality about 90% parsing and 10% finite state machine so if all parsing code is removed there may not be much left
>     
>     2. This is sort-of what I was moving away from because it resulted in the header being parsed again and again each time a specific variable was requested. When it comes to the request-line, to-header and from-header I think we can safely just extract all the supported variables upfront. 
>     
>     There may be other headers that only need to be parsed for certain sip methods. What may be useful is a table that defines the headers that are required for each sip method, then extracting their variables and using the presence of a variable to push an action though the functions rather than visa-versa. Making it more data driven should theoretically increase maintainability
> 
> Nick Lewis wrote:
>     1. Looking at this again I think the name of the function may be misleading. The functions in reqresp_parser.c get the source info. This function is merely storing the source info in the pvt structure. Perhaps something like set_pvt_source() would be a better function name. I do not think it would be beneficial to change the outputs to char** as this would remove the primary purpose of the function. On the subject of testing, since the input and output structures are passed by reference I think it should be possible to unit test it as-is
> 
> David Vossel wrote:
>     I agree with your argument here. set_pvt_source() sounds great.  I think I'm good with this.  Please document the relationship between set_pvt_source and the functions that rely on it so we won't have an confusion about this in the future if stuff gets moved around.  After that I'll give it some testing and try and get it committed.
> 
> Nick Lewis wrote:
>     Looking into this relationship I see that the INVITE, SUBSCRIBE and OPTIONS methods use set_pvt_source. Its presence for OPTIONS may be because get_destination had some from-header parsing in it but it is possible that the variables produced by this from-header parsing were not actually used. I will check.

No my mistake - it is because OPTIONS results in a dialplan lookup for a matching destination extension and a matching source callerid. This seems way overkill for an OPTIONS message. Surely it should just do a find_peer like the NOTIFY method does


- Nick


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


On 2010-06-11 05:53:18, Nick Lewis wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/701/
> -----------------------------------------------------------
> 
> (Updated 2010-06-11 05:53:18)
> 
> 
> Review request for Asterisk Developers and David Vossel.
> 
> 
> Summary
> -------
> 
> The handlers in chan_sip.c for invite and subscription requests parse the from-header twice. The first time is in check_user_full and the second time is in get_destination. This change removes the parsing from these functions and does it beforehand (with an abnf compliant parser)
> 
> 
> This addresses bug 16897.
>     https://issues.asterisk.org/view.php?id=16897
> 
> 
> Diffs
> -----
> 
>   trunk/channels/chan_sip.c 268968 
>   trunk/channels/sip/reqresp_parser.c 268968 
> 
> Diff: https://reviewboard.asterisk.org/r/701/diff
> 
> 
> Testing
> -------
> 
> Compiles, runs, passes associated unit test. CID field appears correct in sip show channel 
> 
> 
> Thanks,
> 
> Nick
> 
>