[asterisk-dev] [Code Review] 2670: ARI: Bridge Playback and Record functions

[jrose]

> On July 17, 2013, 5:39 p.m., rmudgett wrote:
> > /trunk/include/asterisk/channel.h, line 1432
> > <https://reviewboard.asterisk.org/r/2670/diff/2/?file=41370#file41370line1432>
> >
> >     This really should be made a void function since this is really a destructor.  Noone cares about the return value except some CDR and CEL test code that can easily be changed.

I agree with the sentiment below that this should be a separate issue.


> On July 17, 2013, 5:39 p.m., rmudgett wrote:
> > /trunk/main/channel.c, lines 2640-2644
> > <https://reviewboard.asterisk.org/r/2670/diff/2/?file=41375#file41375line2640>
> >
> >     Since you have made this NULL tolerant, the safe_channel_hangup() in test_stasis_endpoints.c can be removed.
> >     
> >     I think the changes to ast_hangup() and the resulting cleanup should be done in a separate patch that this review depends upon.

This review is somewhat time sensitive and there is already another issue that is also time sensitive that is dependant on it, so I don't think making this issue have a further dependency is the right approach right now. I believe we should just file an issue to handle these things and continue as is for now.


- jrose


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


On July 12, 2013, 9:23 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2670/
> -----------------------------------------------------------
> 
> (Updated July 12, 2013, 9:23 p.m.)
> 
> 
> Review request for Asterisk Developers, David Lee, kmoore, Matt Jordan, and rmudgett.
> 
> 
> Bugs: ASTERISK-21592 and ASTERISK-21593
>     https://issues.asterisk.org/jira/browse/ASTERISK-21592
>     https://issues.asterisk.org/jira/browse/ASTERISK-21593
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> The diff has become somewhat wide reaching, but this review primarily takes care of the following:
> 
> 1. Provides a new channel driver for creating unreal channels with specific roles already applied to them and a mechanism for shoving the ;2 end of that channel into a bridge
> 2. Provides Playback and Record on Bridge ARI functions which make use of that channel
> 
> In the process of working on this a few bugs got in the way and this review takes care of those as well.
> 
> 1. Confbridge announcer channels have a reference leak and don't get destroyed on leaving the bridge
> 2. Numerous ARI function documentation items were flawed and others didn't match the intended implementation and had to be tweaked.
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/confbridge/conf_chan_announce.c 394203 
>   /trunk/channels/chan_bridge_media.c PRE-CREATION 
>   /trunk/include/asterisk/bridging.h 394203 
>   /trunk/include/asterisk/channel.h 394203 
>   /trunk/include/asterisk/core_unreal.h 394203 
>   /trunk/include/asterisk/stasis_app.h 394203 
>   /trunk/include/asterisk/stasis_app_playback.h 394203 
>   /trunk/main/bridging.c 394203 
>   /trunk/main/channel.c 394203 
>   /trunk/main/core_unreal.c 394203 
>   /trunk/res/res_stasis.c 394203 
>   /trunk/res/res_stasis_http_bridges.c 394203 
>   /trunk/res/res_stasis_http_channels.c 394203 
>   /trunk/res/res_stasis_http_playback.c 394203 
>   /trunk/res/res_stasis_playback.c 394203 
>   /trunk/res/stasis/control.c 394203 
>   /trunk/res/stasis_http/ari_model_validators.h 394203 
>   /trunk/res/stasis_http/ari_model_validators.c 394203 
>   /trunk/res/stasis_http/resource_bridges.h 394203 
>   /trunk/res/stasis_http/resource_bridges.c 394203 
>   /trunk/res/stasis_http/resource_channels.c 394203 
>   /trunk/rest-api/api-docs/bridges.json 394203 
>   /trunk/rest-api/api-docs/channels.json 394203 
>   /trunk/rest-api/api-docs/playback.json 394203 
>   /trunk/rest-api/api-docs/recordings.json 394203 
> 
> Diff: https://reviewboard.asterisk.org/r/2670/diff/
> 
> 
> Testing
> -------
> 
> All ARI playback and recording functions on channels were tested to make sure they pass validation after the changes I've made
> All ARI playback and recording functions on bridges were tested for functionality in softmix bridges
> 
> I also did some rudimentary tests to see that memory wasn't leaking in the files I touched under these normal conditions. It's hardly exhaustive, but it's a start.
> 
> 
> Thanks,
> 
> jrose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130717/de45112e/attachment.htm>