[asterisk-dev] [Code Review]: ConfbridgeActionExec AMI Command

[Matt Jordan]

> On May 11, 2012, 9:50 a.m., Joshua Colp wrote:
> > /trunk/include/asterisk/bridging.h, lines 167-168
> > <https://reviewboard.asterisk.org/r/1902/diff/1/?file=27723#file27723line167>
> >
> >     I'm not a huge fan of calling this 'deferds'. Perhaps 'deferred' instead?

"deferds" is definitely wrong, since "deferred" is spelled with two r's.  How about deferreds?  I prefer a plural form, since it may contain more then one deferred actions to execute.


> On May 11, 2012, 9:50 a.m., Joshua Colp wrote:
> > /trunk/include/asterisk/bridging.h, line 598
> > <https://reviewboard.asterisk.org/r/1902/diff/1/?file=27723#file27723line598>
> >
> >     I would prefer that if optional data is passed that a destructor be required, doing so forces reinforces that the ownership is with the bridge. (Or if it shouldn't be with the bridge that a reference counted object is used).

Sounds good to me.  I'm hesitant to change bridge_channel_call_deferred to not check for the existence of the destructor callback before calling it on pvt_data, as I'd prefer a memory leak over a seg fault.  I have gone ahead and updated it so that it will spit out a warning if pvt_data is defined but destructor_cb is not. Fixed.


> On May 11, 2012, 9:50 a.m., Joshua Colp wrote:
> > /trunk/main/bridging.c, lines 835-836
> > <https://reviewboard.asterisk.org/r/1902/diff/1/?file=27724#file27724line835>
> >
> >     You can actually just use AST_LIST_REMOVE_HEAD here as it returns the item removed.

Fixed


> On May 11, 2012, 9:50 a.m., Joshua Colp wrote:
> > /trunk/main/bridging.c, lines 1128-1135
> > <https://reviewboard.asterisk.org/r/1902/diff/1/?file=27724#file27724line1128>
> >
> >     You can turn this into a while loop using AST_LIST_REMOVE_HEAD.

Fixed


> On May 11, 2012, 9:50 a.m., Joshua Colp wrote:
> > /trunk/main/bridging.c, lines 1566-1575
> > <https://reviewboard.asterisk.org/r/1902/diff/1/?file=27724#file27724line1566>
> >
> >     Use find_bridge_channel

Fixed


> On May 11, 2012, 9:50 a.m., Joshua Colp wrote:
> > /trunk/main/bridging.c, line 839
> > <https://reviewboard.asterisk.org/r/1902/diff/1/?file=27724#file27724line839>
> >
> >     Is it actually possible for us to go into a deferred execution state without any deferred items? Reading the code it doesn't seem like it is possible.

Well, AST_LIST_REMOVE_HEAD can be executed on an empty list, so you aren't guaranteed that what the macro produces is non-NULL.

Taking the rest of the changes into account, you shouldn't currently be able to get in a situation where deferred is NULL.  However, that doesn't mean that someone a year from now won't change ast_bridge_issue_deferred (or some other related part of the code), and suddenly it is possible to have a NULL item removed from the list.  Another way this could occur is if someone changes the state of the bridged channel to AST_BRIDGE_CHANNEL_STATE_EXECUTE_DEFERRED in an unexpected way and this method gets called twice on a single deferred callback request.  Again, shouldn't be possible, but checking that you got a valid object off a queue when multiple threads are involved with the manipulation of that object isn't a bad idea.


- Matt


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


On May 5, 2012, 11:50 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1902/
> -----------------------------------------------------------
> 
> (Updated May 5, 2012, 11:50 p.m.)
> 
> 
> Review request for Asterisk Developers and Joshua Colp.
> 
> 
> Summary
> -------
> 
> Currently, most actions that are executed upon a channel in a conference (increasing/decreasing listening volume, playing back a sound on a channel, etc.) are handled via DTMF menus associated with a user profile.  While this suffices for scenarios where a user initiates an action upon themselves, it does not account for scenarios where a third party application wants to interact with the channels in a ConfBridge.  While there are a handful of AMI commands that allow for external interaction with the channels in the conference, they are currently limited to a subset of the defined actions.
> 
> Rather then add individual AMI commands for each confbridge action, this patch adds a single new AMI command for ConfBridge, ConfbridgeActionExec.  The command lets you execute any of the ConfBridge actions on a channel in the conference.
> 
> In order to facilitate this, a new mechanism has been added to the bridging layer to allow for a callback function to be executed at the next convenient moment on a bridged channel's thread.  This lets a user of the bridging layer defer execution of some function until such a time that the bridging layer determines that it is safe to execute that action on the channel's thread.
> 
> Example Usage:
> 
> Action: ConfbridgeActionExec
> Conference: 1
> Channel: Local/blah at foo
> Actions: increase_listening_volume,playback(tt-monkeys)
> 
> This would playback monkeys to the offending channel.  Very loudly.
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 364579 
>   /trunk/apps/app_confbridge.c 364788 
>   /trunk/main/bridging.c 364579 
>   /trunk/apps/confbridge/conf_config_parser.c 364579 
>   /trunk/apps/confbridge/include/confbridge.h 364579 
>   /trunk/include/asterisk/bridging.h 364579 
> 
> Diff: https://reviewboard.asterisk.org/r/1902/diff
> 
> 
> Testing
> -------
> 
> Initial testing using a test in the Asterisk Test Suite verified proper behavior of the AMI command.  The channel was properly suspended from the bridging layer, the playback confbridge action was executed, and the channel was placed back into the bridging layer.  All of this was similar to what would occur if the same action was triggered using a DTMF menu.
> 
> Note that a test case is being written to handle the various actions, but will be posted under a separate review.
> 
> 
> Thanks,
> 
> Matt
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120512/68c7cd1f/attachment-0001.htm>