[asterisk-dev] [Code Review] rfc compliant sip option parsing + new unit test

[David Vossel]

> On 2010-06-07 10:42:43, Nick Lewis wrote:
> > /trunk/channels/sip/reqresp_parser.c, line 1514
> > <https://reviewboard.asterisk.org/r/680/diff/1/?file=10422#file10422line1514>
> >
> >     consider moving bit profile to an int *profile and using the return value for indicating parsing failures

This would normally make sense to me, but in this case I don't really know what it would accomplish.  The return value indicates everything the function finds in the options string.  If the options string is zero length, the return value will be 0.  If the parser encounters something it doesn't understand, the return value will have the OPT_UNKNOWN bit turned on and that value will be added to the unsupported string.


> On 2010-06-07 10:42:43, Nick Lewis wrote:
> > /trunk/channels/sip/reqresp_parser.c, line 1590
> > <https://reviewboard.asterisk.org/r/680/diff/1/?file=10422#file10422line1590>
> >
> >     Consider including a name for the subtest as per your other unit tests

done


> On 2010-06-07 10:42:43, Nick Lewis wrote:
> > /trunk/channels/sip/reqresp_parser.c, line 1660
> > <https://reviewboard.asterisk.org/r/680/diff/1/?file=10422#file10422line1660>
> >
> >     Hey this sort of iterate through testcases and check expected result could do with being pulled out into a dedicated unit test helper function since it appears in almost all tests. I guess the testcase structure would need to identify which fields were descriptive, which were function inputs and which were expected function outputs as well as the callback function itself but since unit tests seem to be popular at the moment it may be worth considering

This could be done, but I'm not sure how useful it would be.  What does creating a new unit test feature to add items to a list, iterate through them, and compare the results really do for us?  We'd still have to generate all the test data structures, then register them with some new iterating test object that we made, and then provide some sort of custom callback function to determine how to interpret if the data's test results are considered a pass or failure.  Unless there is a simpler way I am overlooking, that doesn't seem to be any easier than just creating a list and added the items ourselves.


> On 2010-06-07 10:42:43, Nick Lewis wrote:
> > /trunk/channels/sip/reqresp_parser.c, line 1587
> > <https://reviewboard.asterisk.org/r/680/diff/1/?file=10422#file10422line1587>
> >
> >     Consider including this into testdata structure as char **input_unsupported and then have .input_unsupported = &unsupported or .input_unsupported = NULL in the test cases

If we were using some sort of helper function to process these structures that would make sense, but since they all exist in the same function this doesn't really help us.


> On 2010-06-07 10:42:43, Nick Lewis wrote:
> > /trunk/channels/chan_sip.c, line 20220
> > <https://reviewboard.asterisk.org/r/680/diff/1/?file=10419#file10419line20220>
> >
> >     This comment is now some distance from the action. Perhaps it could be moved to reqresp_parser.c or sip.h t oreduce the risk of it getting out of step with reality

I just removed this comment all together, it isn't even necessary now.


- David


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


On 2010-06-01 13:59:40, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/680/
> -----------------------------------------------------------
> 
> (Updated 2010-06-01 13:59:40)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> RFC 3261 section 8.2.2.3 states that if any unsupported options are found in the Require header field, a "420 (Bad Extension)" response should be sent with an Unsupported header field containing only the unsupported options.
> 
> This is not currently being done correctly.  Right now, if Asterisk detects any unsupported sip options in a Require header the entire list of options are returned in the Unsupported header even if some of those options are in fact supported.  This patch fixes that by building an unsupported options character buffer when parsing the options that can be sent with the 420 response.  A unit test verifying this functionality has been created.  Some code refactoring was required.
> 
> RFC 3261 section 8.2.2.3
> "  If a UAS does not understand an option-tag listed in a
>    Require header field, it MUST respond by generating a response with
>    status code 420 (Bad Extension).  The UAS MUST add an Unsupported
>    header field, and list in it those options it does not understand
>    amongst those in the Require header field of the request."
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 266582 
>   /trunk/channels/sip/include/reqresp_parser.h 266582 
>   /trunk/channels/sip/include/sip.h 266582 
>   /trunk/channels/sip/reqresp_parser.c 266582 
> 
> Diff: https://reviewboard.asterisk.org/r/680/diff
> 
> 
> Testing
> -------
> 
> unit test passes and verifies expected behavior... test calls were made as well just as a sanity check.
> 
> 
> Thanks,
> 
> David
> 
>