[asterisk-dev] [Code Review]: Allow specifying which MixMonitor to stop

[Kevin Fleming]

> On Dec. 22, 2011, 8:29 a.m., jrose wrote:
> > It looks like this is working specifically with the filename variable. I'm sorry to say that while the documentation doesn't actually reflect this, filename is only a conditionally required variable.  It's possible for mixmonitor to be started with just an optional r(file) or a t(file) or both. With that in mind, you are probably going to need to come up with a different means of indexing the mixmonitors.
> 
> telecos82 wrote:
>     Sorry, I wasn't aware about these two options, I first developed this code for 1.4 and ported to the trunk :-). Have you got any idea or suggestion about what to use as uid? I think we could use filename variable whenever it is informed and in the rest of the cases the filename associated to r or t options (giving priority to one over the other in case both are informed). What do you think about this approach?
> 
> jrose wrote:
>     Yes, I think that would be the best option. Favor the mixed filename over the others and then r followed by t just because that's the alphabetical order of the options.

This is a common trap to fall into; you're trying to ascribe a different semantic to data that already has one defined. This will end up failing in some way, and should be avoided. Instead, I'd suggest something like adding an option to MixMonitor that allows you to supply it a channel variable name; when the MixMonitor operation succeeds, that variable gets populated with some sort of token (which only needs to be unique for that channel, but it does no harm if it's more globally unique) which can then be supplied to StopMixMonitor to stop that specific instance.

In other words, if you need a tag/token/etc. to refer to a specific instance, then create one; don't try to repurpose something else.


- Kevin


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


On Dec. 22, 2011, 12:03 p.m., telecos82 wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1643/
> -----------------------------------------------------------
> 
> (Updated Dec. 22, 2011, 12:03 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Currently, if there are several MixMonitor started on a channel, there is no way to specify which MixMonitor to stop with StopMixMonitor. With this patch we will allow this. One limitation of current implementation was that mixmonitor datastore was not identified with a uid so there was no way to identify a concrete MixMonitor. Furthermore, when retrieving audiohook to dettach from channel, there was no control on which audiohook to dettach, it always got first audiohook found of type mixmonitor_spy_type. With this patch we are identifying MixMonitor datastore by the filename asociated to it, and dettaching the concrete audiohook contained on datastore. Furthermore a new CLI command "mixmonitor list <channel_name>" to help querying active mixmonitors on a channel.
> 
> 
> This addresses bug ASTERISK-19096.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19096
> 
> 
> Diffs
> -----
> 
>   http://svn.asterisk.org/svn/asterisk/trunk/apps/app_mixmonitor.c 348737 
> 
> Diff: https://reviewboard.asterisk.org/r/1643/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> telecos82
> 
>

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