[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/#review2250
-----------------------------------------------------------


I still think the CLI output is a bit odd. I assume it looks like this:

START  channels/chan_sip/ - sip_parse_options_test
test, * passed got expected unsupported: * and bit profile: *
test, * passed got expected unsupported: * and bit profile: *
test, * passed got expected unsupported: * and bit profile: *
test, * passed got expected unsupported: * and bit profile: *
test, * passed got expected unsupported: * and bit profile: *
test, * passed got expected unsupported: * and bit profile: *
test * with NULL output buf passed, bit profile: *
test * with NULL output buf passed, bit profile: *
test * with NULL output buf passed, bit profile: *
test * with NULL output buf passed, bit profile: *
test * with NULL output buf passed, bit profile: *
test * with NULL output buf passed, bit profile: *
END    channels/chan_sip/ - sip_parse_options_test Time: <1ms Result: PASS

1 Test(s) Executed  1 Passed  0 Failed


I would have preferred a distinction between the one test and the 12 subtests e.g.:

START  channels/chan_sip/ - sip_parse_options_test
Subtest: * passed - got expected unsupported: * and bit profile: *
Subtest: * passed - got expected unsupported: * and bit profile: *
Subtest: * passed - got expected unsupported: * and bit profile: *
Subtest: * passed - got expected unsupported: * and bit profile: *
Subtest: * passed - got expected unsupported: * and bit profile: *
Subtest: * passed - got expected unsupported: * and bit profile: *
Subtest: * with NULL output buf passed - got bit profile: *
Subtest: * with NULL output buf passed - got bit profile: *
Subtest: * with NULL output buf passed - got bit profile: *
Subtest: * with NULL output buf passed - got bit profile: *
Subtest: * with NULL output buf passed - got bit profile: *
Subtest: * with NULL output buf passed - got bit profile: *
END    channels/chan_sip/ - sip_parse_options_test Time: <1ms Result: PASS

1 Test(s) Executed  1 Passed  0 Failed


but no big deal. If your terms of engagement do not also require a digium shipit then I think this can be committed 

- Nick


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