[asterisk-dev] [Code Review] Backport of state_interface for app_queue in Asterisk 1.4

[Mark Michelson]

> On 2009-03-29 13:41:01, Russell Bryant wrote:
> > /branches/1.4/apps/app_queue.c, lines 921-927
> > <http://reviewboard.digium.com/r/116/diff/2/?file=3845#file3845line921>
> >
> >     You're leaking a reference here for every iteration of this loop where the string doesn't match.

Fixed.


> On 2009-03-29 13:41:01, Russell Bryant wrote:
> > /branches/1.4/apps/app_queue.c, line 4422
> > <http://reviewboard.digium.com/r/116/diff/2/?file=3845#file3845line4422>
> >
> >     I believe this is the same thing as ast_skip_blanks().

Yeah, you're right. When I wrote this, I copied the same logic used for the membername and penalty parsing. I'll change them all to use ast_skip_blanks.


> On 2009-03-29 13:41:01, Russell Bryant wrote:
> > /branches/1.4/apps/app_queue.c, line 4901
> > <http://reviewboard.digium.com/r/116/diff/2/?file=3845#file3845line4901>
> >
> >     hm?

I think I spotted a bug when making the backport here and made a note to myself to make sure that the same bug wasn't in Asterisk Business Edition. The comment isn't useful now and probably should have had a 'XXX' on it. I have now removed it.


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/116/#review641
-----------------------------------------------------------


On 2009-03-26 14:12:35, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/116/
> -----------------------------------------------------------
> 
> (Updated 2009-03-26 14:12:35)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This is a backport of functionality that is already in all releases of Asterisk 1.6.
> 
> The functionality being added here is the ability to associate an interface with a queue member which is used to determine that member's device state.
> A common usage for such functionality is to accurately determine the device state of a queue member who is called via the Agent or Local channel 
> technologies. By specifying a more "concrete" interface to watch for device state, the queue application can more accurately decide the availability
> of the queue member.
> 
> The policy for Asterisk 1.4 is to not introduce new functionality, and only make bug fixes. This case is one where the line has been blurred significantly
> and so community feedback would be helpful. From a development standpoint, this violates the policy since most of what is being added is a new option for
> queue members. From many users' perspectives, however, this is a means of solving a deficiency in the basic operations of queues. Please take a look at the
> diff attached and comment on whether this would be problematic. It is implemented in such a way that any old dialplans, CLI commands, manager commands, etc.
> will not need to be updated unless you are making use of this new functionality.
> 
> In internal discussions, the only point that was brought up was a possible problem that could aris if using persistent members. If you upgraded to a version 
> that had the state interface functionality in it and then downgraded to a version which did not have state interface functionality, then the queue members' 
> names loaded from the database would be wrong (they would have a semicolon and the state interface appended). Since the members' names are only used for display 
> purposes, since the error may be easily corrected by reloading the queue configuration,and since the number of people likely to be affected by this is very 
> small, I feel it's a risk worth taking.
> 
> Another potential problem I have thought of is that Asterisk 1.4 does not contain func_devstate. This means that this solution will only work if
> you know for sure what endpoint will be dialed when a call is placed from a queue. To give an example, if you know that calling Local/2000 will always 
> end up dialing SIP/2000, then this functionality will work great for you. If calling Local/2000 may end up dialing SIP/2000 or potentially SIP/3000 based 
> on some dialplan logic, then this new functionality will not help you since you cannot use custom device states in the dialplan.
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/apps/app_queue.c 183027 
> 
> Diff: http://reviewboard.digium.com/r/116/diff
> 
> 
> Testing
> -------
> 
> I used the scenario of issue 12970 as a basis for creating a test.
> 
> For the test, I created an Agent channel, Agent/1000, and had this line in the
> dialplan:
> 
> exten => 1006,1,AgentCallbackLogin(1000,,1000 at internal)
> 
> In the context "internal," extension 1000 is as follows:
> 
> exten => 1000,1,Dial(SIP/1000)
> 
> If it is not clear, All calls to Agent/1000 will eventually go to SIP/1000. I added
> a line to my queues.conf file for my queue:
> 
> member => Agent/1000,,,SIP/1000
> 
> When I place an outbound call from SIP/1000, issuing the "queue show" command shows that
> Agent/1000 is "in use." Attempting to place a call to the queue will not cause SIP/1000 to
> ring since I have ringinuse=no for my queue. When SIP/1000 hangs up, the status returns to 
> "not in use."
> 
> 
> As another test, I tried implementing a common problem. In my queues.conf file, I added this
> to my queue:
> 
> member => Local/1000,,,SIP/1000
> 
> As you may be able to guess, Local/1000 always dials SIP/1000. I placed a call to my queue
> and answered with SIP/1000. At this point, issuing a "queue show" command showed that the
> member Local/1000 was "in use." Then, from SIP/1000 I transferred the call to another endpoint.
> Issuing a "queue show" command then showed Local/1000 as being "not in use."
> 
> 
> Thanks,
> 
> Mark
> 
>