[asterisk-dev] [Code Review] chan_agent: Prevent multiple channels from logging in as the same agent.

[Mark Michelson]

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

Ship it!


This is one of those reviews where clicking the "Ship it!" is incredibly scary. You've modified large portions of the most volatile channel driver in Asterisk, but I can't pinpoint anything wrong with what you've done. Pair that with the fact I know you've been testing this a lot lately, and I'm inclined to go ahead and click it.

- Mark


On Jan. 3, 2013, 4:39 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2260/
> -----------------------------------------------------------
> 
> (Updated Jan. 3, 2013, 4:39 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Multiple channels logging in as the same agent can result in dead channels waiting for a condition signal that will never come because another channel thread stole it.  The symptom is chan_sip repeatedly generating warning messages about rescheduling autodestruction of dialogs with an agent channel owner.
> 
> * Made only login_exec() (the app AgentLogin) clear the agent_pvt->chan pointer to prevent multiple channels from logging in as the same agent.  agent_read(), agent_call(), and agent_set_base_channel() no longer disconnect the agent channel from the agent_pvt.  This also eliminates the need to keep checking for agent_pvt->chan being NULL.
> 
> * Made agent_hangup() not wake up the AgentLogin agent thread until it is done.
> 
> * Made agent_request() not able to get the agent until he has logged in and any wrapup time has expired.
> 
> * Made agent_request() use ast_hangup() instead of agent_hangup() to correctly dispose of a channel.
> 
> * Removed agent_set_base_channel().  Nobody calls it and it is a bad thing in general.
> 
> * Made only agent_devicestate() determine the current device state of an agent.  Note: Agent group device states have never been supported.
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_agent.c 378488 
> 
> Diff: https://reviewboard.asterisk.org/r/2260/diff
> 
> 
> Testing
> -------
> 
> Overnight stress testing no longer has these dead channels.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130122/9590b8a4/attachment.htm>