[asterisk-dev] [Code Review] 3689: Media format improvements: the great renaming + some cleanup

Matt Jordan reviewboard at asterisk.org
Wed Jul 2 17:50:20 CDT 2014 


> On July 2, 2014, 2:55 a.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed-trunk/main/format_cap.c, lines 124-131
> > <https://reviewboard.asterisk.org/r/3689/diff/1/?file=62001#file62001line124>
> >
> >     I'm against duplicating code.  I'd much rather this be moved to a static (inline?) function.
> 
> Matt Jordan wrote:
>     So, the reason why these routines are duplicated is due to having to call the explicit debug version of ao2_* in the debug variant of the function. For example, __ast_format_cap_alloc should *not* call __ao2_alloc_debug, as doing so would be ignoring the compile time REF_DEBUG options.
>     
>     I'm not sure what a better way of handling this would be. Since these routines are relatively short, I'm not sure breaking these up into two or three pieces is necessarily better.

Per IRC, the static inline function should work just fine.


- Matt


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


On July 1, 2014, 9:53 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3689/
> -----------------------------------------------------------
> 
> (Updated July 1, 2014, 9:53 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch performs the renaming discussed on the asterisk-dev list [1]. As usual, as I was going through it some other cleanups occurred, and in order to make sure I didn't make memory leaks worse, some parts of Corey's memory leak cleanups made their way in as well.
> 
> [1] http://lists.digium.com/pipermail/asterisk-dev/2014-June/068133.html
> 
> The great renaming:
> 
>  * ast_format_cap_add             => ast_format_cap_append
>  * ast_format_cap_add_all_by_type => ast_format_cap_append_all_by_type
>  * ast_parse_allow_disallow       => ast_format_cap_update_by_allow_disallow
>  * ast_cap_remove_bytype          => ast_format_cap_remove_by_type
>  * ast_getformatname_multiple     => ast_format_cap_get_names, and now uses an ast_str ** instead of a char *buf/size_t len
>  * ast_format_sdp_generate        => ast_format_generate_sdp_fmtp
>  * ast_format_sdp_parse           => ast_format_parse_sdp_fmtp
> 
> Functions removed:
>  * ast_format_compatibility_get_original_id - no longer used
> 
> Note that chan_h323, chan_gtalk, and chan_jingle did not get all of the re-namings (particularly ast_getformatname_multiple => ast_format_cap_get_names), and will not compile with this patch. These modules should be removed, per the discussion on the -dev list.
> 
> Functions that support REF_DEBUG:
>  * ast_format_cap_alloc
>  * ast_format_cap_append
>  * ast_format_cache_get
> 
> Memory leak cleanups (many of which came from Corey's patches):
>  * Channel technology struct capabilities leak their format capabilities on off nominal exit paths (chan_motif, chan_pjsip, chan_unistim, chan_skinny)
>  * Format capabilities leak in chan_pjsip_new nominal path
>  * Leak of nativeformats format_cap in nominal path of channel.c's ast_channel_alloc
>  * Leak of format_cap in channel.c's set_format
>  * Leak of format_cap in channel.c's ast_request
>  * Usage of ao2_replace in translate.c to avoid overwriting a ref
>  * Clean up of frame format cache in RTP instance in res_rtp_asterisk destructor
> 
> Unit tests
>  * Remove test for ast_getformatname_multiple, add test for ast_format_cap_get_names
> 
> 
> Diffs
> -----
> 
>   /team/group/media_formats-reviewed-trunk/tests/test_voicemail_api.c 417740 
>   /team/group/media_formats-reviewed-trunk/tests/test_format_cap.c 417740 
>   /team/group/media_formats-reviewed-trunk/tests/test_format_api.c 417740 
>   /team/group/media_formats-reviewed-trunk/tests/test_core_format.c 417740 
>   /team/group/media_formats-reviewed-trunk/tests/test_config.c 417740 
>   /team/group/media_formats-reviewed-trunk/res/res_stasis_snoop.c 417740 
>   /team/group/media_formats-reviewed-trunk/res/res_stasis.c 417740 
>   /team/group/media_formats-reviewed-trunk/res/res_rtp_asterisk.c 417740 
>   /team/group/media_formats-reviewed-trunk/res/res_pjsip_sdp_rtp.c 417740 
>   /team/group/media_formats-reviewed-trunk/res/res_pjsip/pjsip_configuration.c 417740 
>   /team/group/media_formats-reviewed-trunk/res/res_clioriginate.c 417740 
>   /team/group/media_formats-reviewed-trunk/res/res_calendar.c 417740 
>   /team/group/media_formats-reviewed-trunk/res/res_agi.c 417740 
>   /team/group/media_formats-reviewed-trunk/res/parking/parking_applications.c 417740 
>   /team/group/media_formats-reviewed-trunk/res/ari/resource_channels.c 417740 
>   /team/group/media_formats-reviewed-trunk/res/ari/resource_bridges.c 417740 
>   /team/group/media_formats-reviewed-trunk/pbx/pbx_spool.c 417740 
>   /team/group/media_formats-reviewed-trunk/main/translate.c 417740 
>   /team/group/media_formats-reviewed-trunk/main/sorcery.c 417740 
>   /team/group/media_formats-reviewed-trunk/main/rtp_engine.c 417740 
>   /team/group/media_formats-reviewed-trunk/main/media_index.c 417740 
>   /team/group/media_formats-reviewed-trunk/main/manager.c 417740 
>   /team/group/media_formats-reviewed-trunk/main/frame.c 417740 
>   /team/group/media_formats-reviewed-trunk/main/format_compatibility.c 417740 
>   /team/group/media_formats-reviewed-trunk/main/format_cap.c 417740 
>   /team/group/media_formats-reviewed-trunk/main/format_cache.c 417740 
>   /team/group/media_formats-reviewed-trunk/main/format.c 417740 
>   /team/group/media_formats-reviewed-trunk/main/file.c 417740 
>   /team/group/media_formats-reviewed-trunk/main/dial.c 417740 
>   /team/group/media_formats-reviewed-trunk/main/core_local.c 417740 
>   /team/group/media_formats-reviewed-trunk/main/config_options.c 417740 
>   /team/group/media_formats-reviewed-trunk/main/cli.c 417740 
>   /team/group/media_formats-reviewed-trunk/main/channel.c 417740 
>   /team/group/media_formats-reviewed-trunk/main/ccss.c 417740 
>   /team/group/media_formats-reviewed-trunk/main/bridge_basic.c 417740 
>   /team/group/media_formats-reviewed-trunk/main/bridge.c 417740 
>   /team/group/media_formats-reviewed-trunk/main/astobj2_container.c 417740 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/slin.h 417740 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/rtp_engine.h 417740 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/frame.h 417740 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/format_compatibility.h 417740 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/format_cap.h 417740 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/format_cache.h 417740 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/format.h 417740 
>   /team/group/media_formats-reviewed-trunk/funcs/func_channel.c 417740 
>   /team/group/media_formats-reviewed-trunk/codecs/ex_ulaw.h 417740 
>   /team/group/media_formats-reviewed-trunk/codecs/ex_gsm.h 417740 
>   /team/group/media_formats-reviewed-trunk/codecs/ex_alaw.h 417740 
>   /team/group/media_formats-reviewed-trunk/codecs/ex_adpcm.h 417740 
>   /team/group/media_formats-reviewed-trunk/channels/pjsip/dialplan_functions.c 417740 
>   /team/group/media_formats-reviewed-trunk/channels/dahdi/bridge_native_dahdi.c 417740 
>   /team/group/media_formats-reviewed-trunk/channels/chan_unistim.c 417740 
>   /team/group/media_formats-reviewed-trunk/channels/chan_skinny.c 417740 
>   /team/group/media_formats-reviewed-trunk/channels/chan_sip.c 417740 
>   /team/group/media_formats-reviewed-trunk/channels/chan_pjsip.c 417740 
>   /team/group/media_formats-reviewed-trunk/channels/chan_phone.c 417740 
>   /team/group/media_formats-reviewed-trunk/channels/chan_oss.c 417740 
>   /team/group/media_formats-reviewed-trunk/channels/chan_nbs.c 417740 
>   /team/group/media_formats-reviewed-trunk/channels/chan_multicast_rtp.c 417740 
>   /team/group/media_formats-reviewed-trunk/channels/chan_motif.c 417740 
>   /team/group/media_formats-reviewed-trunk/channels/chan_misdn.c 417740 
>   /team/group/media_formats-reviewed-trunk/channels/chan_mgcp.c 417740 
>   /team/group/media_formats-reviewed-trunk/channels/chan_jingle.c 417740 
>   /team/group/media_formats-reviewed-trunk/channels/chan_iax2.c 417740 
>   /team/group/media_formats-reviewed-trunk/channels/chan_h323.c 417740 
>   /team/group/media_formats-reviewed-trunk/channels/chan_gtalk.c 417740 
>   /team/group/media_formats-reviewed-trunk/channels/chan_dahdi.c 417740 
>   /team/group/media_formats-reviewed-trunk/channels/chan_console.c 417740 
>   /team/group/media_formats-reviewed-trunk/channels/chan_bridge_media.c 417740 
>   /team/group/media_formats-reviewed-trunk/channels/chan_alsa.c 417740 
>   /team/group/media_formats-reviewed-trunk/bridges/bridge_softmix.c 417740 
>   /team/group/media_formats-reviewed-trunk/bridges/bridge_simple.c 417740 
>   /team/group/media_formats-reviewed-trunk/bridges/bridge_native_rtp.c 417740 
>   /team/group/media_formats-reviewed-trunk/bridges/bridge_holding.c 417740 
>   /team/group/media_formats-reviewed-trunk/apps/confbridge/conf_chan_record.c 417740 
>   /team/group/media_formats-reviewed-trunk/apps/app_voicemail.c 417740 
>   /team/group/media_formats-reviewed-trunk/apps/app_originate.c 417740 
>   /team/group/media_formats-reviewed-trunk/apps/app_meetme.c 417740 
>   /team/group/media_formats-reviewed-trunk/apps/app_dumpchan.c 417740 
>   /team/group/media_formats-reviewed-trunk/apps/app_confbridge.c 417740 
>   /team/group/media_formats-reviewed-trunk/addons/chan_ooh323.c 417740 
>   /team/group/media_formats-reviewed-trunk/addons/chan_mobile.c 417740 
> 
> Diff: https://reviewboard.asterisk.org/r/3689/diff/
> 
> 
> Testing
> -------
> 
> Negotiation using PJSIP still works.
> 
> The most prevalent memory leak that still occurs happens when a statically allocated frame ref bumps a format. This happens a lot when generating translator tables, and in various other places as well.
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

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

More information about the asterisk-dev mailing list