[asterisk-dev] [Code Review] 3519: media_formats: Add legacy format API and move chan_iax2, chan_h323, and chan_misdn over.

[Joshua Colp]

> On May 1, 2014, 6:51 p.m., Kevin Harwell wrote:
> > /team/group/media_formats-reviewed/channels/chan_h323.c, lines 1049-1050
> > <https://reviewboard.asterisk.org/r/3519/diff/1/?file=58191#file58191line1049>
> >
> >     If no longer needed then delete, or did you mean to come back to this?

BUGBUGs added.


> On May 1, 2014, 6:51 p.m., Kevin Harwell wrote:
> > /team/group/media_formats-reviewed/channels/chan_h323.c, lines 2615-2618
> > <https://reviewboard.asterisk.org/r/3519/diff/1/?file=58191#file58191line2615>
> >
> >     More of the same commented code.

Ditto.


> On May 1, 2014, 6:51 p.m., Kevin Harwell wrote:
> > /team/group/media_formats-reviewed/channels/chan_iax2.c, lines 6282-6284
> > <https://reviewboard.asterisk.org/r/3519/diff/1/?file=58192#file58192line6282>
> >
> >     May not be a problem, but what happens if/when these formats get unref'ed since it points to a global object?  If the incoming frame is on the stack the format won't be unref'ed so should be fine, otherwise these might need a ref bump.

It is indeed on the stack and is safe.


> On May 1, 2014, 6:51 p.m., Kevin Harwell wrote:
> > /team/group/media_formats-reviewed/main/format_compatibility.c, lines 308-317
> > <https://reviewboard.asterisk.org/r/3519/diff/1/?file=58205#file58205line308>
> >
> >     It looks possible that this could return NULL.  Looking through some of the code I saw a few spots where this function was called but the NULL result was not checked for and there would be a possibility of a NULL pointer being dereffed.  So do those places need a NULL check, should this return some kind of empty format representing NULL, or is this something that really should never happen or are all the cases calling this know based on the passed in params that NULL won't be returned?

I've looked over the channel drivers that are using this (chan_iax2/chan_h323) and both either check the value directly or do check getting a NULL return value. Can you point to some specific cases?


- Joshua


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


On April 30, 2014, 10:54 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3519/
> -----------------------------------------------------------
> 
> (Updated April 30, 2014, 10:54 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This change adds a legacy legacy format compatibility API which is used by chan_iax2, chan_h323, and chan_misdn to work in new media formats land.
> 
> 
> Diffs
> -----
> 
>   /team/group/media_formats-reviewed/main/format_compatibility.c PRE-CREATION 
>   /team/group/media_formats-reviewed/main/codec_builtin.c 413144 
>   /team/group/media_formats-reviewed/include/asterisk/translate.h 413144 
>   /team/group/media_formats-reviewed/include/asterisk/format_compatibility.h PRE-CREATION 
>   /team/group/media_formats-reviewed/include/asterisk/format_cache.h 413144 
>   /team/group/media_formats-reviewed/include/asterisk/codec.h 413144 
>   /team/group/media_formats-reviewed/codecs/codec_dahdi.c 413144 
>   /team/group/media_formats-reviewed/channels/iax2/provision.c 413144 
>   /team/group/media_formats-reviewed/channels/iax2/parser.c 413144 
>   /team/group/media_formats-reviewed/channels/h323/chan_h323.h 413144 
>   /team/group/media_formats-reviewed/channels/h323/ast_h323.cxx 413144 
>   /team/group/media_formats-reviewed/channels/chan_phone.c 413144 
>   /team/group/media_formats-reviewed/channels/chan_misdn.c 413144 
>   /team/group/media_formats-reviewed/channels/chan_iax2.c 413144 
>   /team/group/media_formats-reviewed/channels/chan_h323.c 413144 
>   /team/group/media_formats-reviewed/apps/app_meetme.c 413144 
> 
> Diff: https://reviewboard.asterisk.org/r/3519/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

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