[asterisk-dev] [Code Review] Update to chan_unistim functionality

Matt Jordan reviewboard at asterisk.org
Thu Mar 1 09:20:37 CST 2012 

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

Ship it!


I reviewed this mostly for coding guideline infractions.  I've given this a Ship It, subject to resolving a few issues:
* Fix the ref counting leak, noted below
* As this is a *massive* change, with lots of new functionality, both CHANGES and UPGRADE need to be updated with the new features and modifications to functionality.

All of this could use doxygen comments, but that's an improvement you could make outside of this patch.

Great work, and thanks for being willing to maintain this module!



/trunk/channels/chan_unistim.c
<https://reviewboard.asterisk.org/r/1243/#comment10419>

    As an improvement, you may want to comment these structs.  I'm not sure if the char* can be converted to length limited char[] or string fields, but that's a possible improvement as well



/trunk/channels/chan_unistim.c
<https://reviewboard.asterisk.org/r/1243/#comment10422>

    Check here to see if the ao2_container_alloc fails



/trunk/channels/chan_unistim.c
<https://reviewboard.asterisk.org/r/1243/#comment10420>

    This could use an error message - if a language configuration file doesn't exist, that's probably worth noting



/trunk/channels/chan_unistim.c
<https://reviewboard.asterisk.org/r/1243/#comment10421>

    Clarify this error message - if one of these just appeared in the CLI, people would be fairly confused.  



/trunk/channels/chan_unistim.c
<https://reviewboard.asterisk.org/r/1243/#comment10423>

    This needs to be fixed before the patch goes in.  The ao2_find is going to bump the ref count of lang_entry by one - since you're returning str_trans, you probably want to deref the lang_entry count before you do that, otherwise you could end up in a situation where the ref count on the lang_entry objects keeps increasing, preventing their destruction when the underlying container is disposed of



/trunk/channels/chan_unistim.c
<https://reviewboard.asterisk.org/r/1243/#comment10424>

    Globally, you may want to statements protected by unistimdebug ast_debug statements - however, that's an improvement that doesn't need to be made with this patch



/trunk/channels/chan_unistim.c
<https://reviewboard.asterisk.org/r/1243/#comment10425>

    Blob



/trunk/channels/chan_unistim.c
<https://reviewboard.asterisk.org/r/1243/#comment10426>

    Check for success of allocation before using it here



/trunk/channels/chan_unistim.c
<https://reviewboard.asterisk.org/r/1243/#comment10427>

    If unistim_new fails, c would most likely be NULL here.  You probably should check if c is NULL prior to this point.



/trunk/channels/chan_unistim.c
<https://reviewboard.asterisk.org/r/1243/#comment10428>

    Is there a reason why this is commented out?  If it should be left in the source, you may want to at least put a comment explaining why; otherwise remove it



/trunk/contrib/unistimLang/en.po
<https://reviewboard.asterisk.org/r/1243/#comment10431>

    Fill in this information.
    
    As this is a new type of configuration file, it may be worth specifying in a comment in this file how the file is configured



/trunk/contrib/unistimLang/ru.po
<https://reviewboard.asterisk.org/r/1243/#comment10432>

    Same comments as the previous file



/trunk/contrib/unistimLang/ru.po.utf8
<https://reviewboard.asterisk.org/r/1243/#comment10433>

    And this one needs a header


- Matt


On Feb. 28, 2012, 11:02 a.m., IgorG wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1243/
> -----------------------------------------------------------
> 
> (Updated Feb. 28, 2012, 11:02 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> New unistim.conf options:
> - Added "debug" global option in unistim.conf, that enable debug when module loaded
> - Added "sharpdial" option, enable sending call whet # key pressed
> 
> New features:
> - ability for changing display language (tested on Russian language). Use .po files in encoding, able to display
>   ISO 8859-1, ISO 8859-2, ISO 8859-4, ISO 8859-5, ISO 2022-JP. For selecting language can be used option "language" in
>   unistim.conf or screen menu.
> - Support for multilines
> - Support for holding multiple lines
> - More fixes for display on i2002 phone
> - Configurable keys for sending and received history
> - Menu for selecting codec, contrast (not yet completed) or display language
> - Show clock at first line of idle phone
> - Add ability for pick up call
> - Pick up call by using on-screen soft key
> - Change displaying list of received or send calls (callerid, time and caller name on different screens, listed by lef-right keys)
> 
> Changes:
> - Changed entering on screen phone number, so any number of digits can be entered
> - rtp_port now used start rtp port
> - list of dial tone frequecies now loaded from indications.conf and not hardcoded
> - Key with globe icon how calls menu and not directly codec selection
> 
> Fixes:
> - 0017406 Correct updating LED when switching between speekerphone and handset or hanging up
> - 0017327 Multiple crashes when using phone
> - 0016867 Fixed playing dialtone in some scenarious when conversation already started
> - Fixed dispalying on-screen information when using Redial softkey (DN number and timer displayed).
> - Not sending short ring in case of call forward enabled on phone
> 
> 
> This addresses bug 18229.
>     https://issues.asterisk.org/jira/browse/18229
> 
> 
> Diffs
> -----
> 
>   /trunk/configs/unistim.conf.sample 357253 
>   /trunk/contrib/unistimLang/en.po PRE-CREATION 
>   /trunk/contrib/unistimLang/ru.po PRE-CREATION 
>   /trunk/contrib/unistimLang/ru.po.utf8 PRE-CREATION 
>   /trunk/channels/chan_unistim.c 357253 
> 
> Diff: https://reviewboard.asterisk.org/r/1243/diff
> 
> 
> Testing
> -------
> 
> Testing done by issue tracker users: ibercom, scsiborg, idarwin, TeknoJuce, c0rnoTa. 
> Tested on production system by Jonn Taylor (jonnt) using phone models: Nortel i2004, 1120E and 1140E.
> 
> 
> Thanks,
> 
> IgorG
> 
>

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

More information about the asterisk-dev mailing list