[asterisk-dev] [Code Review] SIP: refactoring parsing functions and the addition of new unit tests

[David Vossel]

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


I've had some second thoughts about the refactoring structure.  I'm going to take this review down for now until I am able to make the necessary changes.

- David


On 2010-01-30 16:39:51, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/477/
> -----------------------------------------------------------
> 
> (Updated 2010-01-30 16:39:51)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> ----- Changes -----
> 
> New files
> - sip.h – A new header for shared #define, enum, and struct definitions.
> - config-parser.c  – Contains sip.conf parsing helper functions with unit tests.
> - request-parser.c – Contains sip request and response parsing helper functions with unit tests.
> 
> New Unit Tests 
> - sip_parse_uri_test
> - sip_parse_host_test
> - sip_parse_register_line_test
> 
> Code Refactoring
> - All reusable #define, enum, and struct definitions were moved out of chan_sip.c into sip.h. During this process formatting changes were made to comments in both sip.h and chan_sip.c in order to better adhere to the coding guidelines.
> - Function prototypes for functions shared among chan_sip.c and the new request-parser.c and config-parser.c files were added to sip.h.
> - parse_uri() and get_calleridname() were moved from chan_sip.c to request-parser.c along with unit tests for both functions.
> - sip_parse_host() and sip_parse_register_line() were moved from chan_sip.c to config-parser.c along with unit tests for both functions.
> 
> Changes to parse_uri()
> -removal of the options parameter.  It was never used and did not behave correctly.
> -additional check for [?header] field. When this field was present, the transport type was not being set correctly.
> 
> ----- Overview -----
> 
> This patch is introduced with the hope that unit tests for all our sip parsing functions will be written soon.  chan_sip is a huge file, and with the addition of each unit test chan_sip is going to grow larger and harder to maintain.  I'm proposing we begin refactoring chan_sip, starting with the parsing functions.  With each parsing function we move into a separate helper file, a unit test should accompany it.  I've attempted to lay down the ground work for this change by creating two new parser helper files (config-parser.c and request-parser.c) and moving all shared structs, enums, and defines from chan_sip.c into a shared sip.h file.  We can't verify everything in Asterisk using unit tests, but string parsing is one area where unit tests make the most sense.  By beginning to restructure the code in this way, chan_sip not only becomes less bloated, but Asterisk as a whole will become more stable.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/Makefile 244062 
>   /trunk/channels/chan_sip.c 244062 
>   /trunk/channels/sip/config-parser.c PRE-CREATION 
>   /trunk/channels/sip/request-parser.c PRE-CREATION 
>   /trunk/channels/sip/sip.h PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/477/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> David
> 
>