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

[Russell Bryant]

> On 2010-09-03 09:11:23, Russell Bryant wrote:
> > I don't think I like this change.  This is quite a change in behavior for Local channels.
> > 
> > For this issue in app_queue, for other channel types, the issue is solved by being able to set the default language for devices in sip.conf and other channel driver configuration files.  Obviously, this won't work for chan_local.  It could be a queue agent configuration parameter.  Perhaps more generally, it could be a Local channel dial string option like L(<language>).
> 
> Terry Wilson wrote:
>     Yes, the problem is only for local channels. The problem is that this is how Local channels *should* work. And the problem isn't necessarily just for queues. One shouldn't have to jump through a lot of hoops just to get things to work. Especially with /n channels, we are conceptually saying "this is a channel". Having Set(CHANNEL()=...) happen in the dialplan and not effect that conceptual channel just seems wrong to me. One side of the local channel inherits all of the settings of the other at creation, but the two do not remain linked when they are updated. This just makes sure that that happens (for the specific case of CHANNEL() writes).
>     
>     I really can't think of a way that this would actually cause a problem. Of course, that could just be my limited imagination. Perhaps we could try to think of instances where this might break things?
> 
> Terry Wilson wrote:
>     Also, it isn't just about language. There's musicclass, callgroup, etc. Lots of different settings. You don't want to have to string together 10 different dial options.

Yeah, I thought about the fact that other options are affected, too.  My next thought was some sort of setvar option like S(CHANNEL(language)=fr) that could be used more than once in a Local channel dial string.

I see where you're coming from with this, and you may be right.  Let me see if I can come up with a real example of how this could be a problem.  If I can't, we can just proceed with what you have proposed.


- Russell


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


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
> 
>