[asterisk-dev] [Code Review] 3734: media formats: Add a 'none' format to prevent NULL format derefences; update unit tests

[Corey Farrell]

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


I did not do a good review of any of the new unit tests.


/team/group/media_formats-reviewed-trunk/main/bridge_channel.c
<https://reviewboard.asterisk.org/r/3734/#comment22819>

    Check still needed or can we just assert?



/team/group/media_formats-reviewed-trunk/main/bridge_channel.c
<https://reviewboard.asterisk.org/r/3734/#comment22820>

    Check still needed or can we just assert?



/team/group/media_formats-reviewed-trunk/main/codec_builtin.c
<https://reviewboard.asterisk.org/r/3734/#comment22821>

    I'm not sure AUDIO is appropriate.  Maybe UNKNOWN is better, or even a new AST_MEDIA_TYPE_NULL?



/team/group/media_formats-reviewed-trunk/main/format_cache.c
<https://reviewboard.asterisk.org/r/3734/#comment22823>

    This procedure could previously unset a format:
    ast_format_cache_set("ulaw", NULL);
    
    Is that not needed?
    
    Also ast_format_get_name is called many times, it would be better to call it once and save to char *name;



/team/group/media_formats-reviewed-trunk/main/format_cache.c
<https://reviewboard.asterisk.org/r/3734/#comment22824>

    I think just assert for !format.



/team/group/media_formats-reviewed-trunk/main/format_cache.c
<https://reviewboard.asterisk.org/r/3734/#comment22825>

    No need to check this if ast_format_cache_set will not accept NULL's.



/team/group/media_formats-reviewed-trunk/main/format_cache.c
<https://reviewboard.asterisk.org/r/3734/#comment22826>

    If we won't support !format then simplify this.



/team/group/media_formats-reviewed-trunk/tests/test_format_cache.c
<https://reviewboard.asterisk.org/r/3734/#comment22827>

    Assuming we will just assert in ast_format_cache_set then this test has to go away.  Also the ast_test_status_update message inaccurate - "with a name".  NULL format can't have a name.


- Corey Farrell


On July 9, 2014, 8:37 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3734/
> -----------------------------------------------------------
> 
> (Updated July 9, 2014, 8:37 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch does two things:
> 
> * It updates a few of the unit tests for some of the API changes. In particular, it focuses on adding some tests for formats with attributes and their expected behaviour. A few other non-format related unit tests were updated as well to handle off nominals detected during testing.
> 
> * It adds an 'ast_format_none' format. This format is a dummy format that can be used instead of a NULL pointer to prevent having to put NULL dereference checks into every place in the codebase. Channels are no assigned this format immediately upon creation, and their default capabilities are set to have it. As this format's codec has no translation (nor a representation in the RTP engine), it _shouldn't_ cause harm.
> 
> * A few NULL checks were put in anyway into key areas in a few modules. These were ones that were hit hard by the unit tests and prone to crashing if presented a NULL format.
> 
> 
> Diffs
> -----
> 
>   /team/group/media_formats-reviewed-trunk/tests/test_format_cap.c 418259 
>   /team/group/media_formats-reviewed-trunk/tests/test_format_cache.c 418259 
>   /team/group/media_formats-reviewed-trunk/tests/test_core_format.c 418259 
>   /team/group/media_formats-reviewed-trunk/tests/test_cel.c 418259 
>   /team/group/media_formats-reviewed-trunk/main/format_cap.c 418259 
>   /team/group/media_formats-reviewed-trunk/main/format_cache.c 418259 
>   /team/group/media_formats-reviewed-trunk/main/codec_builtin.c 418259 
>   /team/group/media_formats-reviewed-trunk/main/codec.c 418259 
>   /team/group/media_formats-reviewed-trunk/main/channel.c 418259 
>   /team/group/media_formats-reviewed-trunk/main/bridge_channel.c 418259 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/format_cache.h 418259 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/codec.h 418259 
> 
> Diff: https://reviewboard.asterisk.org/r/3734/diff/
> 
> 
> Testing
> -------
> 
> Unit tests pass.
> 
> There is a FRACK on shutdown, but it doesn't appear to be caused by this patch (things didn't run long enough without this patch before)
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140710/4cfc9a40/attachment-0001.html>