[asterisk-dev] [Code Review]: Named call pickup groups. Fixes, missing functionality, and improvements.

[rmudgett]

> On Sept. 14, 2012, 12:33 a.m., Alec Davis wrote:
> > first look seems ok. 
> > 
> > Just nit picking.
> > Yet to test it.
> > Surprised that multiple calls being pickup by multiple extensions failed, I could have sworn that it worked when call_pickup was reworked.

I could see the scenario in the code.  If the oldest candidate found is no longer available, the pickup would just give up even though the search may have found more than one candidate.  This patch fixes it so it looks for the next oldest candidate until it is out of candidates.


> On Sept. 14, 2012, 12:33 a.m., Alec Davis wrote:
> > /trunk/main/channel.c, line 8323
> > <https://reviewboard.asterisk.org/r/2112/diff/2/?file=31225#file31225line8323>
> >
> >     spelling: specified

Fixed.


> On Sept. 14, 2012, 12:33 a.m., Alec Davis wrote:
> > /trunk/main/channel.c, line 8590
> > <https://reviewboard.asterisk.org/r/2112/diff/2/?file=31225#file31225line8590>
> >
> >     why not 
> >     	for ((it = ao2_iterator_init(grp, 0)); (ng = ao2_iterator_next(&it)); ao2_ref(ng, -1)) { 
> >     
> >     In other words, the orignal was easier to read, and achieved the same.
> >     
> >     The a02_iterator_next is evaluated before the loop is executed.
> >

Using the for loop gets you thinking about what is going on.
For all items in the container do...

I initially put the iterator init there but put it before the for loop to shorten the line length.
I moved it back to the for init section since it fits the line length guideline.


- rmudgett


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


On Sept. 13, 2012, 6:52 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2112/
> -----------------------------------------------------------
> 
> (Updated Sept. 13, 2012, 6:52 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> * ASTERISK-20383
> Missing named call pickup group features:
> 
> CHANNEL(callgroup) - Need CHANNEL(namedcallgroup)
> CHANNEL(pickupgroup) - Need CHANNEL(namedpickupgroup)
> Pickup() - Needs to also select from named pickup groups.
> 
> * ASTERISK-20384
> Using the pickupexten, the pickup channel selection could fail even though there was a call it could have picked up. In a call pickup race when there are multiple calls to pickup and two extensions try to pickup a call, it is conceivable that the loser will not pick up any call even though it could have picked up the next oldest matching call.
> 
> Regression because of the named call pickup group feature.
> 
> * See ASTERISK-20386 for the improvements.
> 
> * Fixed some locking issues in CHANNEL().
> 
> 
> This addresses bugs ASTERISK-20383, ASTERISK-20384 and ASTERISK-20386.
>     https://issues.asterisk.org/jira/browse/ASTERISK-20383
>     https://issues.asterisk.org/jira/browse/ASTERISK-20384
>     https://issues.asterisk.org/jira/browse/ASTERISK-20386
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_directed_pickup.c 373055 
>   /trunk/funcs/func_channel.c 373055 
>   /trunk/include/asterisk/channel.h 373055 
>   /trunk/include/asterisk/features.h 373055 
>   /trunk/main/channel.c 373055 
>   /trunk/main/features.c 373055 
> 
> Diff: https://reviewboard.asterisk.org/r/2112/diff
> 
> 
> Testing
> -------
> 
> Tested call pickupexten, Pickup(), CHANNEL(namedcallgroup), and CHANNEL(namedpickupgroup) for named call group functionality.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

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