[asterisk-dev] [Code Review] 3388: media_formats: Move chan_mgcp, chan_unistim, and chan_skinny over.

[Mark Michelson]

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



/team/group/media_formats-reviewed/channels/chan_mgcp.c
<https://reviewboard.asterisk.org/r/3388/#comment21046>

    If you fail to allocate caps here, should you abandon the read?



/team/group/media_formats-reviewed/channels/chan_mgcp.c
<https://reviewboard.asterisk.org/r/3388/#comment21045>

    The structure used here is problematic. This else can be reached either if the channel was not allocated or if the format caps could not be allocated.
    
    If the error was that the channel was allocated but the format caps were not, then you need to free the channel and NULL it. Otherwise, you'll be returning some messed up channel pointer.
    
    This warning message should be amended. The problem may be that the channel could not be allocated, or it may be that the format caps couldn't be allocated.
    
    I suggest altering the structure of this function like so:
    
    tmp = <allocate_channel>
    if (!tmp) {
       warn that channel couldn't be created.
       return NULL;
    }
    caps = <allocate caps>
    if (!caps) {
       warn that caps couldn't be created.
       free tmp;
       return NULL;
    }
    <big block of code that used to be indented and that assumes tmp and caps are both non-NULL>



/team/group/media_formats-reviewed/channels/chan_mgcp.c
<https://reviewboard.asterisk.org/r/3388/#comment21047>

    I was about to comment that the check for if the format is compatible with p->cap is missing, but since we're iterating over the formats in p->cap, that should always be true, right?



/team/group/media_formats-reviewed/channels/chan_mgcp.c
<https://reviewboard.asterisk.org/r/3388/#comment21048>

    Hm, but on this one, you've left in the ast_format_cap_iscompatible_format call. I'm a bit confused on why you've removed it in some places but not others.


- Mark Michelson


On March 25, 2014, 11:11 a.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3388/
> -----------------------------------------------------------
> 
> (Updated March 25, 2014, 11:11 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This moves the chan_mgcp, chan_unistim, and chan_skinny modules over to the media formats API.
> 
> 
> Diffs
> -----
> 
>   /team/group/media_formats-reviewed/include/asterisk/format_cache.h 411016 
>   /team/group/media_formats-reviewed/include/asterisk/channel.h 410188 
>   /team/group/media_formats-reviewed/channels/chan_unistim.c 409286 
>   /team/group/media_formats-reviewed/channels/chan_skinny.c 409286 
>   /team/group/media_formats-reviewed/channels/chan_mgcp.c 409286 
> 
> Diff: https://reviewboard.asterisk.org/r/3388/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

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