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

[Russell Bryant]

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


This is looking good so far.  The changes look a lot more scary than they really are since so much code had to be moved.

I would second Mark's idea that a nice improvement to the tests would be to provide different types of invalid input, as well.  However, if that were to come as a future improvement, I'm okay with that, too.

I would also like to see just a bit of additional testing on top of running the unit tests.  Go ahead and configure a registration, make a call, etc., to make sure you exercise each of these API calls that got moved to make sure that nothing big blew up in the changes you had to make in chan_sip.  I know the risk is low, but it's a pretty quick test to do.


/trunk/channels/sip/config_parser.c
<https://reviewboard.asterisk.org/r/477/#comment3264>

    s/domin/domain/ ... not that it really matters



/trunk/channels/sip/config_parser.c
<https://reviewboard.asterisk.org/r/477/#comment3265>

    I suppose this is just a suggestion for future improvement, but it would be really nice to have sip_registry_alloc() and sip_registry_destroy() functions instead of having to write in the details of allocation, initialization, and destruction multiple times in the code.


- Russell


On 2010-02-02 12:22:49, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/477/
> -----------------------------------------------------------
> 
> (Updated 2010-02-02 12:22:49)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> ----- Changes -----
> New files
> - channels/sip/sip.h – A new header for shared #define, enum, and struct definitions.
> - channels/sip/include/sip-utils.h – sip util functions shared among the all the sip APIs
> - channels/sip/include/config-parser.h – sip config-parser API
> - channels/sip/config-parser.c  – Contains sip.conf parsing helper functions with unit tests.
> - channels/sip/include/reqresp-parser.h – sip request response parser API
> - channels/sip/reqresp-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.
> - The beginnings of three new sip APIs, sip-utils.h, config-parser.h, reqresp-parser.h using existing chan_sip.c functions.
> - 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 reqresp-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.
> 
> 
> UPDATE:  This is an update to my original review request.  The major change made here is that each new .c file in the sip/ directory now has it's own header file in sip/include/.  I did this because I do not think it is a good idea to start putting function prototypes into the main sip.h file.  We should really be breaking up these functions into separate APIs.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/Makefile 244328 
>   /trunk/channels/chan_sip.c 244328 
>   /trunk/channels/sip/config_parser.c PRE-CREATION 
>   /trunk/channels/sip/include/config_parser.h PRE-CREATION 
>   /trunk/channels/sip/include/reqresp_parser.h PRE-CREATION 
>   /trunk/channels/sip/include/sip.h PRE-CREATION 
>   /trunk/channels/sip/include/sip_utils.h PRE-CREATION 
>   /trunk/channels/sip/reqresp_parser.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/477/diff
> 
> 
> Testing
> -------
> 
> compiled and ran unit tests.
> 
> 
> Thanks,
> 
> David
> 
>