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

[Nick Lewis]

> 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
> 
> David Vossel wrote:
>     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.

Agreed - this suggestion was mostly because of the embryonic unit test helper idea. With such a helper, the functions under test would probably want to have a consistent format such as 
error = functionname(input,&output[1],&output[n])
so that the helper could easily test for error and see the profile as output[1] so it could simply compare output array with expected array
Without the helper this is of no benefit


> 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
> 
> David Vossel wrote:
>     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.

It is easier in languages such as Verilog and VHDL in which functions have a more defined structure with parameters designated with directions such as in, out and inout. Pass or failure can be determined generically by comparing all the actual outputs with the expected outputs and those languages provide built-in support for such testing. To be honest if it were really simple I would probably have done it myself in tests for parse_name_andor_addr etc


- Nick


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


On 2010-06-08 17:12:28, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/680/
> -----------------------------------------------------------
> 
> (Updated 2010-06-08 17:12:28)
> 
> 
> 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 269118 
>   /trunk/channels/sip/include/reqresp_parser.h 269118 
>   /trunk/channels/sip/include/sip.h 269118 
>   /trunk/channels/sip/reqresp_parser.c 269118 
> 
> 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
> 
>