[asterisk-dev] [Code Review] Extended maximum number of pickupgroups (callgroup/pickupgroup)

[Mark Michelson]

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


Be sure when compiling that you are compiling with dev-mode enabled. When you run the configure script, be sure to add --enable-dev-mode as a command line option. This will result in more warnings being checked and will treat warnings as errors. There are a few places throughout where errors are reported for me when I try to compile this code.

Everything I've pointed out below is very small. For the most part, the code looks good.


/trunk/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/2043/#comment12728>

    This function would probably work better if it just returned a struct timeval instead of a struct timeval pointer.



/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/2043/#comment12719>

    ast_str_hash() is a better hash function than ast_hashtab_hash_string().
    
    If named groups are intended to be case-insensitive, then you may consider using ast_str_case_hash() instead.



/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/2043/#comment12720>

    You can replace this if statement with ao2_cleanup(groups);



/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/2043/#comment12721>

    You can replace this if statement with ao2_cleanup(groups);



/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/2043/#comment12722>

    There is a reference leak in this loop. Be sure to ao2_ref(ng, -1); at the end of each iteration.



/trunk/main/features.c
<https://reviewboard.asterisk.org/r/2043/#comment12725>

    Coding guidelines require curly braces for single line if statements.



/trunk/main/features.c
<https://reviewboard.asterisk.org/r/2043/#comment12724>

    Coding guidelines require curly braces for single line if statements.



/trunk/main/features.c
<https://reviewboard.asterisk.org/r/2043/#comment12723>

    I suggest altering this to be
    
    match = ao2_callback(a, 0, namedgroup_match, b);
    
    Using ao2_callback_data is unnecessary here. Just pass b as the 'arg' parameter.



/trunk/main/features.c
<https://reviewboard.asterisk.org/r/2043/#comment12726>

    You don't need the cast here.


- Mark


On July 13, 2012, 8:18 a.m., Guenther Kelleter wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2043/
> -----------------------------------------------------------
> 
> (Updated July 13, 2012, 8:18 a.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson, Matt Jordan, and Thomas Arimont.
> 
> 
> Summary
> -------
> 
> This patch adds the feature "Extended maximum number of pickupgroups (callgroup/pickupgroup)".
> 
> The feature is implemented as named pickup/call groups which can be used in parallel to but independent from the already known numbered call/pickup groups. Named groups will allow an unlimited number of pickup/call groups.
> Named groups are configured with the keywords "namedcallgroup" and "namedpickupgroup" (conforming to "callgroup" and "pickupgroup").
> A namedpickupgroup "4" does not match the callgroup number 4.
> 
> Additionally the behavior of the undirected pickup is changed so that the oldest ringing channel (creation time of the channel) is picked up when more than one channel could be picked up. This will be required by another new feature which is yet to be posted.
> 
> A few words to describe the implementation:
> Names of named groups are collected in a container at configuration time of a user. This container will be copied (in fact referenced) around to the channels in parallel with the numbered groups bitfield.
> When an undirected pickup is executed, the pickup and call groups are matched against each other, and the oldest target channel which matches is chosen. Notably there is almost no overhead for the named group handling when no pickup is executed, except referencing of the containers.
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 370052 
>   /trunk/channels/chan_dahdi.c 370052 
>   /trunk/channels/chan_misdn.c 370052 
>   /trunk/channels/chan_sip.c 370052 
>   /trunk/channels/misdn/chan_misdn_config.h 370052 
>   /trunk/channels/misdn_config.c 370052 
>   /trunk/channels/sip/include/sip.h 370052 
>   /trunk/configs/chan_dahdi.conf.sample 370052 
>   /trunk/configs/misdn.conf.sample 370052 
>   /trunk/configs/sip.conf.sample 370052 
>   /trunk/include/asterisk/channel.h 370052 
>   /trunk/main/channel.c 370052 
>   /trunk/main/channel_internal_api.c 370052 
>   /trunk/main/features.c 370052 
> 
> Diff: https://reviewboard.asterisk.org/r/2043/diff
> 
> 
> Testing
> -------
> 
> Call pickup tested for sip, dahdi and misdn channels. Reference counter handling of ao2_container (which includes the group names) checked with additional debug output.
> 
> 
> Thanks,
> 
> Guenther
> 
>

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