[asterisk-dev] [Code Review] Allow inheritance of CHANNEL() write changes for Local/n queue members

[Terry Wilson]

> On 2010-09-03 11:35:07, Russell Bryant wrote:
> > /branches/1.4/funcs/func_channel.c, line 104
> > <https://reviewboard.asterisk.org/r/903/diff/1/?file=12382#file12382line104>
> >
> >     I think you need to initialize loop to 0.  I also wonder if there is a clearer way to write this.  Maybe ...
> >     
> >     unsigned int loop_iterations = 2;
> >     
> >     do {
> >     
> >     } while (--loop_iterations && .. )

doh, yeah. I had it initialized in a previous version and moved stuff around and forgot to put it back. My initial version included a goto. :-p


> On 2010-09-03 11:35:07, Russell Bryant wrote:
> > /branches/1.4/include/asterisk/channel.h, lines 292-295
> > <https://reviewboard.asterisk.org/r/903/diff/1/?file=12383#file12383line292>
> >
> >     I haven't been able to think of a problem of the approach in general.  However, I really don't think it's wise to change the ast_channel_tech API in released versions.  You could probably abuse the getoption() callback to accomplish what you need.
> >     
> >     However, a bigger implementation concern is the safety of reaching over to the other side of the Local channel.  To guarantee that channel is valid, you'll need some crazy locking.
> >     
> >     You would need to ensure that get_other_side is called without a channel lock held.  In the the callback, you need to lock local_pvt.  Finally, you will need to lock the other side channel before returning it, which will involve some deadlock avoidance.
> >     
> >     :-(

grr. :-)


- Terry


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


On 2010-09-03 09:05:00, Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/903/
> -----------------------------------------------------------
> 
> (Updated 2010-09-03 09:05:00)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Having Local (/n) channels as queue members and setting the language in the extension with Set(CHANNEL(language)=fr) sets the language on the Local/...,2 channel. Hold time report playbacks happen on the Local/...,1 channel and therefor do not play in the specified language.
> 
> This patch adds a channel callback function get_other_side() (I am totally open to another name for this) that given one side of a local channel, will pass back the other side. This is used in the func_channel_write function to apply any changes to both sides of the local channel.
> 
> There are other places in the code where we specifically check for a local channel and look up the other side of the channel via relying on the names of the channels being similar and doing an ast_get_channel_by_name_locked(), but that is just exceedingly gross. We can't use the get_base_channel() callback because that could have some unintended consequences during a masquerade.
> 
> I propose that we make this change in all supported release versions, despite it being an API change, since there really is no other way for a local (/n) channel in a queue to set the desired language without *some* kind of fix--and this is the least hacky way I could come up with.
> 
> 
> This addresses bug 17673.
>     https://issues.asterisk.org/view.php?id=17673
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/channels/chan_local.c 284782 
>   /branches/1.4/funcs/func_channel.c 284782 
>   /branches/1.4/include/asterisk/channel.h 284782 
> 
> Diff: https://reviewboard.asterisk.org/r/903/diff
> 
> 
> Testing
> -------
> 
> ;queues.conf
> [test]
> strategy=ringall
> member => Local/s at queuetest/n
> reportholdtime=yes
> 
> ;extensions.conf
> [sip]
> exten => 123,1,Queue(test)
> 
> [queuetest]
> exten => s,1,Set(CHANNEL(language)=fr)
> exten => s,n,Dial(SIP/6004)
> 
> The hold time notification played in french.
> 
> 
> Thanks,
> 
> Terry
> 
>