[asterisk-dev] [Code Review] reworked chan_ooh323

Jeff Peeler jpeeler at digium.com
Mon Nov 2 15:30:30 CST 2009 

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


In general the "always" suggestions made are applicable everywhere. You said that you would fix the formatting in the future, it needs fixing even in the new code you wrote. But that effort should be alright to address over time. Please clean up all the commented out code and address the issues (small ones it seems) that you have marked with your name.


/trunk/addons/chan_ooh323.c
<https://reviewboard.asterisk.org/r/324/#comment2844>

    should always be using ast_free everywhere allocated with an ast allocation



/trunk/addons/chan_ooh323.c
<https://reviewboard.asterisk.org/r/324/#comment2834>

    always should be using ast_malloc, i guess whenever not using functions from memheap.c.



/trunk/addons/chan_ooh323.c
<https://reviewboard.asterisk.org/r/324/#comment2839>

    doesn't look like the channel needs to be hung up here at least



/trunk/addons/chan_ooh323.c
<https://reviewboard.asterisk.org/r/324/#comment2842>

    can't wait for the monitor call thread in this fashion - waiting on a pthread condition may be required



/trunk/addons/chan_ooh323.c
<https://reviewboard.asterisk.org/r/324/#comment2843>

    yeah, returning 0 here seems fine



/trunk/addons/chan_ooh323.c
<https://reviewboard.asterisk.org/r/324/#comment2845>

    why not just use ast_strupda and remove the free?



/trunk/addons/chan_ooh323.c
<https://reviewboard.asterisk.org/r/324/#comment2846>

    %abled



/trunk/addons/chan_ooh323.c
<https://reviewboard.asterisk.org/r/324/#comment2847>

    always shorten this with the DEADLOCK_AVOIDANCE macro



/trunk/addons/chan_ooh323.c
<https://reviewboard.asterisk.org/r/324/#comment2848>

    why was this change made?



/trunk/addons/chan_ooh323.c
<https://reviewboard.asterisk.org/r/324/#comment2849>

    Well if there's no use, remove this.



/trunk/addons/chan_ooh323.c
<https://reviewboard.asterisk.org/r/324/#comment2850>

    this 512 is used several different places, maybe worth making a define



/trunk/addons/ooh323c/src/memheap.c
<https://reviewboard.asterisk.org/r/324/#comment2853>

    if this is safe to assume, remove the else part



/trunk/addons/ooh323c/src/ooCapability.c
<https://reviewboard.asterisk.org/r/324/#comment2854>

    I think you should just return false here if epCap->cap != OO_T38 and remove cap and the conditional outside the switch



/trunk/addons/ooh323c/src/ooCapability.c
<https://reviewboard.asterisk.org/r/324/#comment2855>

    typo



/trunk/addons/ooh323c/src/ooCmdChannel.c
<https://reviewboard.asterisk.org/r/324/#comment2856>

    this may not be the place to note this, but is there any reason why CmdChanLock is a void pointer?



/trunk/addons/ooh323c/src/ooGkClient.c
<https://reviewboard.asterisk.org/r/324/#comment2857>

    unlock



/trunk/addons/ooh323c/src/ooGkClient.c
<https://reviewboard.asterisk.org/r/324/#comment2858>

    unlock again



/trunk/addons/ooh323c/src/ooGkClient.c
<https://reviewboard.asterisk.org/r/324/#comment2860>

    Perhaps locking should be added here. I haven't traced the code path to check and see.



/trunk/addons/ooh323c/src/ooSocket.c
<https://reviewboard.asterisk.org/r/324/#comment2861>

    Instead of casting, it'd be nice to just make the function parameter size type socklen_t if possible



/trunk/addons/ooh323cDriver.c
<https://reviewboard.asterisk.org/r/324/#comment2852>

    ast_pthread_create_detached_background, and remove the pthread attr init and destroy


- Jeff


On 2009-10-29 12:10:28, may213 wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/324/
> -----------------------------------------------------------
> 
> (Updated 2009-10-29 12:10:28)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> There is reworked chan_ooh323. 
> 
> Architecture changes are:
> multi-thread instead of single-thread original version. One thread is for commands from asterisk call thread to h323 stack, one thread is for accepting endpoint connection and talking with gatekeeper, one thread per call is for call processing.
> separate memory context from one endpoint context to many contexts of one per call. mutexes for locking critical data are introduced.
> 
> Protocol changes are:
> fast start response is generated based on our capability instead of return copy of remote FS OLC
> fast start response is generated one time per call
> capability adaptation to remote side framing
> improved gatekeeper support
> non-standart codec (G726 and G726r32 from Cisco, AMB NB)  support
> t.38 and mode changes (request mode/request mode ack) support
> 
> T.38 support, which implemented in two modes. Transparent mode is for non-t38 aware channels where translation from t38 packet to sound made by chan_ooh323 himself and faxgw mode which is t.38 pass-through and compatible with xFax applications and chan_sip.
> 
> and many small and cosmetic changes.
> 
> 
> Diffs
> -----
> 
>   /trunk/addons/Makefile 226527 
>   /trunk/addons/chan_ooh323.h 226527 
>   /trunk/addons/chan_ooh323.c 226527 
>   /trunk/addons/ooh323c/src/context.c 226527 
>   /trunk/addons/ooh323c/src/decode.c 226527 
>   /trunk/addons/ooh323c/src/dlist.c 226527 
>   /trunk/addons/ooh323c/src/encode.c 226527 
>   /trunk/addons/ooh323c/src/errmgmt.c 226527 
>   /trunk/addons/ooh323c/src/eventHandler.h 226527 
>   /trunk/addons/ooh323c/src/eventHandler.c 226527 
>   /trunk/addons/ooh323c/src/h323/H235-SECURITY-MESSAGESDec.c 226527 
>   /trunk/addons/ooh323c/src/h323/H323-MESSAGESDec.c 226527 
>   /trunk/addons/ooh323c/src/h323/MULTIMEDIA-SYSTEM-CONTROLDec.c 226527 
>   /trunk/addons/ooh323c/src/memheap.h 226527 
>   /trunk/addons/ooh323c/src/memheap.c 226527 
>   /trunk/addons/ooh323c/src/ooCalls.h 226527 
>   /trunk/addons/ooh323c/src/ooCalls.c 226527 
>   /trunk/addons/ooh323c/src/ooCapability.h 226527 
>   /trunk/addons/ooh323c/src/ooCapability.c 226527 
>   /trunk/addons/ooh323c/src/ooCmdChannel.h 226527 
>   /trunk/addons/ooh323c/src/ooCmdChannel.c 226527 
>   /trunk/addons/ooh323c/src/ooCommon.h 226527 
>   /trunk/addons/ooh323c/src/ooDateTime.h 226527 
>   /trunk/addons/ooh323c/src/ooDateTime.c 226527 
>   /trunk/addons/ooh323c/src/ooGkClient.h 226527 
>   /trunk/addons/ooh323c/src/ooGkClient.c 226527 
>   /trunk/addons/ooh323c/src/ooLogChan.h 226527 
>   /trunk/addons/ooh323c/src/ooLogChan.c 226527 
>   /trunk/addons/ooh323c/src/ooSocket.h 226527 
>   /trunk/addons/ooh323c/src/ooSocket.c 226527 
>   /trunk/addons/ooh323c/src/ooStackCmds.h 226527 
>   /trunk/addons/ooh323c/src/ooStackCmds.c 226527 
>   /trunk/addons/ooh323c/src/ooTimer.c 226527 
>   /trunk/addons/ooh323c/src/ooUtils.c 226527 
>   /trunk/addons/ooh323c/src/ooasn1.h 226527 
>   /trunk/addons/ooh323c/src/oochannels.h 226527 
>   /trunk/addons/ooh323c/src/oochannels.c 226527 
>   /trunk/addons/ooh323c/src/ooh245.h 226527 
>   /trunk/addons/ooh323c/src/ooh245.c 226527 
>   /trunk/addons/ooh323c/src/ooh323.h 226527 
>   /trunk/addons/ooh323c/src/ooh323.c 226527 
>   /trunk/addons/ooh323c/src/ooh323ep.h 226527 
>   /trunk/addons/ooh323c/src/ooh323ep.c 226527 
>   /trunk/addons/ooh323c/src/ooports.c 226527 
>   /trunk/addons/ooh323c/src/ooq931.h 226527 
>   /trunk/addons/ooh323c/src/ooq931.c 226527 
>   /trunk/addons/ooh323c/src/ootrace.h 226527 
>   /trunk/addons/ooh323c/src/ootrace.c 226527 
>   /trunk/addons/ooh323c/src/ootypes.h 226527 
>   /trunk/addons/ooh323c/src/perutil.c 226527 
>   /trunk/addons/ooh323c/src/printHandler.h 226527 
>   /trunk/addons/ooh323c/src/printHandler.c 226527 
>   /trunk/addons/ooh323c/src/rtctype.c 226527 
>   /trunk/addons/ooh323cDriver.h 226527 
>   /trunk/addons/ooh323cDriver.c 226527 
> 
> Diff: https://reviewboard.asterisk.org/r/324/diff
> 
> 
> Testing
> -------
> 
> Tested on heavily-loaded h323 transit server with call rate up to 8 per sec and up to 200k per day. asterisk restarted only for changing module. No memory leaks.
> 
> T.38 tested in both mode. It work in transparent mode for sending and receiving faxes, work in faxgw mode with Send/ReceiveFax applications. Faxing with chan_sip is not tested because i don't have sip terminal with t.38 support.
> 
> 
> Thanks,
> 
> may213
> 
>

More information about the asterisk-dev mailing list