[asterisk-dev] [Code Review] 3625: media_formats: Update most of core (main/*.c)

[Matt Jordan]

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


Very nice work!


/team/group/media_formats-reviewed/main/core_unreal.c
<https://reviewboard.asterisk.org/r/3625/#comment22292>

    Given the number of off nominal paths *and* the fact that this will be de-ref'd on the nominal path, you may want to consider using RAII_VAR here.
    
    It has been used inappropriately in other places, but this one may make some sense.



/team/group/media_formats-reviewed/main/core_unreal.c
<https://reviewboard.asterisk.org/r/3625/#comment22291>

    This is one of those weird-isms about core_unreal:
    
    While its pvt is ao2 ref counted, the channel core does not expect a channel's pvt to be a ref counted object. Hence, if you destruct a channel and that channel still has a pvt, it will attempt to ast_free the pvt.
    
    Since the channel was just allocated here, that means this will destroy the channel and cause a memory corruption. (Yup, there's a bunch of places where this happens in here - whoops)
    
    In the off nominal paths, make sure you call ast_channel_tech_pvt_set(owner, NULL) - or move the association of the owner/pvt later on in the code.



/team/group/media_formats-reviewed/main/data.c
<https://reviewboard.asterisk.org/r/3625/#comment22305>

    Is/was fr_len used anywhere else?



/team/group/media_formats-reviewed/main/format_compatibility.c
<https://reviewboard.asterisk.org/r/3625/#comment22306>

    As pedantic as it may seem, I'd convert BUG to BUGBUG.
    
    "BUG" shows up too much for grep to be useful. BUGBUG has been rather helpful in finding things that need to get completed before the code is considered "finished"


- Matt Jordan


On June 19, 2014, 9:50 a.m., Corey Farrell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3625/
> -----------------------------------------------------------
> 
> (Updated June 19, 2014, 9:50 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Updates to allow most of the Asterisk core to compile.  I've excluded main/channel.c, main/dsp.c and main/rtp_engine.c.  Changes to those files will be posted separate since I feel they are more complex and likely to have more error's.  If any of the files included in this review fit that description let me know and I will split them off.
> 
> This change does not include any replacement for calls to ast_format_is_slinear(), and adds it back to the header (but does not implement).  So ast_format_is_slinear hasn't been fixed, just deferred to become a link error.
> 
> The modifications to chan_phone are to allow what I believe to be a comparability function to be in the correct namespace to be implemented in format_compatibility.c.
> 
> 
> Diffs
> -----
> 
>   /team/group/media_formats-reviewed/main/stasis_channels.c 416235 
>   /team/group/media_formats-reviewed/main/sounds_index.c 416235 
>   /team/group/media_formats-reviewed/main/sorcery.c 416235 
>   /team/group/media_formats-reviewed/main/slinfactory.c 416235 
>   /team/group/media_formats-reviewed/main/media_index.c 416235 
>   /team/group/media_formats-reviewed/main/manager.c 416235 
>   /team/group/media_formats-reviewed/main/indications.c 416235 
>   /team/group/media_formats-reviewed/main/image.c 416235 
>   /team/group/media_formats-reviewed/main/frame.c 416235 
>   /team/group/media_formats-reviewed/main/format_pref.c 416235 
>   /team/group/media_formats-reviewed/main/format_compatibility.c 416235 
>   /team/group/media_formats-reviewed/main/format.c 416235 
>   /team/group/media_formats-reviewed/main/file.c 416235 
>   /team/group/media_formats-reviewed/main/dial.c 416235 
>   /team/group/media_formats-reviewed/main/data.c 416235 
>   /team/group/media_formats-reviewed/main/core_unreal.c 416235 
>   /team/group/media_formats-reviewed/main/core_local.c 416235 
>   /team/group/media_formats-reviewed/main/codec.c 416235 
>   /team/group/media_formats-reviewed/include/asterisk/slinfactory.h 416235 
>   /team/group/media_formats-reviewed/include/asterisk/rtp_engine.h 416235 
>   /team/group/media_formats-reviewed/include/asterisk/format_pref.h 416235 
>   /team/group/media_formats-reviewed/include/asterisk/format_compatibility.h 416235 
>   /team/group/media_formats-reviewed/include/asterisk/format_cache.h 416235 
>   /team/group/media_formats-reviewed/include/asterisk/format.h 416235 
>   /team/group/media_formats-reviewed/include/asterisk/file.h 416235 
>   /team/group/media_formats-reviewed/channels/chan_phone.c 416235 
> 
> Diff: https://reviewboard.asterisk.org/r/3625/diff/
> 
> 
> Testing
> -------
> 
> Compiled.
> 
> 
> Thanks,
> 
> Corey Farrell
> 
>

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