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

[Nick Lewis]

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



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/680/#comment4613>

    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



/trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/680/#comment4618>

    consider moving bit profile to an int *profile and using the return value for indicating parsing failures 



/trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/680/#comment4616>

    Consider including this into testdata structure as char **input_unsupported and then have .input_unsupported = &unsupported or .input_unsupported = NULL in the test cases



/trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/680/#comment4619>

    Consider including a name for the subtest as per your other unit tests



/trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/680/#comment4621>

    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



/trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/680/#comment4617>

    Could be rolled into main test loop if using .input_unsupported = NULL It probably only needs a couple of tests here rather than repeating them all


- Nick


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