[asterisk-dev] [Code Review] Convert ast_channel over to astobj2

Mark Michelson mmichelson at digium.com
Mon Mar 23 14:46:46 CDT 2009 

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/203/#review594
-----------------------------------------------------------


Third pass...


/trunk/include/asterisk/channel.h
<http://reviewboard.digium.com/r/203/#comment1473>

    s/simple/simply/



/trunk/main/channel.c
<http://reviewboard.digium.com/r/203/#comment1483>

    The doxygen states that chan must be locked prior to calling ast_change_name. If that is the case, this recursive locking is unnecessary.



/trunk/main/cli.c
<http://reviewboard.digium.com/r/203/#comment1491>

    A bit of an optimization suggestion: If the command is "core show channels count" there is no need to use an iterator at all. You can get the job done with a call to ast_active_channels() and ast_active_calls().



/trunk/main/cli.c
<http://reviewboard.digium.com/r/203/#comment1492>

    It's not clear why you have used ast_channel_iterator_all_new here instead of ast_channel_iterator_by_name_new.



/trunk/res/res_agi.c
<http://reviewboard.digium.com/r/203/#comment1499>

    Missing a call to ast_channel_unref here.



/trunk/res/res_agi.c
<http://reviewboard.digium.com/r/203/#comment1501>

    The logic in this function isn't bulletproof.
    
    Say that an AGI is running on channel SIP/1000-12345678. If the AGI calls
    
    "get full variable HANGUPCAUSE SIP/1000-12345678" then because there is a channel name given as an argument, chan2 will have a reference to chan. Then, at the end of the function, since chan2 and chan are pointers to the same channel, it means that we will leak a reference.
    
    There are a few ways to handle this. One way is always make it so that chan2 will need to be unreffed at the end of the function. Another way is to change the condition by which chan2 is unreffed to be if argc == 5.



/trunk/res/res_clioriginate.c
<http://reviewboard.digium.com/r/203/#comment1502>

    It's not really necessary to lock the channel since it will be locked further down in the function calls of ast_aync_parseable_goto.



/trunk/res/res_monitor.c
<http://reviewboard.digium.com/r/203/#comment1506>

    The only places requiring the channel lock to be held are when we use c->name to create fname and when calling ast_monitor_setjoinfiles. The rest of the code either isn't modifying the channel's data, or in the case of ast_monitor_start, it will lock the channel for us.



/trunk/res/res_monitor.c
<http://reviewboard.digium.com/r/203/#comment1504>

    You don't need to lock the channel here since ast_monitor_stop does it, too.
    
    Or if you just really wanted to lock and unlock the channel here, you could change the second parameter to ast_monitor_stop to be 0 instead so that we don't recursively lock the channel.



/trunk/res/res_monitor.c
<http://reviewboard.digium.com/r/203/#comment1505>

    This is another case where locking the channel is not necessary since ast_monitor_change_fname will lock the channel for us.



/trunk/res/res_monitor.c
<http://reviewboard.digium.com/r/203/#comment1507>

    No need to lock c since ast_monitor_pause and ast_monitor_unpause will lock c for you.


- Mark


On 2009-03-21 12:27:36, Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/203/
> -----------------------------------------------------------
> 
> (Updated 2009-03-21 12:27:36)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch converts handling of the ast_channel data structure over to the astobj2 framework.  There is a lot that could be said about this, but the patch is a bit improvement for performance, stability, code maintainability, and ease of future code development.
> 
> The channel list is no longer an unsorted linked list.  The main container for channels is an astobj2 hash table.  All of the code related to searching for channels or iterating active channels has been rewritten.  Let n be the number of active channels.  Iterating the channel list has gone from O(n^2) to O(n).  Searching for a channel by name went from O(n) to O(1).  Searching for a channel by extension is still O(n), but uses a new method for doing so, which is more efficient.
> 
> The ast_channel object is now a reference counted object.  The benefits here are plentiful.  Some benefits directly related to issues in the previous code include:
> 
> 1) When threads other than the channel thread owning a channel wanted access to a channel, it had to hold the lock on it to ensure that it didn't go away.  This is no longer a requirement.  Holding a reference is sufficient.
> 
> 2) There are places that now require less dealing with channel locks.
> 
> 3) There are places where channel locks are held for much shorter periods of time.
> 
> 4) There are places where dealing with more than one channel at a time becomes _MUCH_ easier.  ChanSpy is a great example of this.  Writing code in the future that deals with multiple channels will be much easier.
> 
> Some additional information regarding channel locking and reference count handling can be found in channel.h, where a new section has been added that discusses some of the rules associated with it.
> 
> Mark Michelson also assisted with the development of this patch.  He did the conversion of ChanSpy and introduced a new API, ast_autochan, which makes it much easier to deal with holding on to a channel pointer for an extended period of time and having it get automatically updated if the channel gets masqueraded.
> 
> One final note is that currently, app_dahdichan will not compile.  I just haven't been motivated enough to do it.  Everything else should be good to go, though.  I think the best thing to do is to just add whatever is necessary to ChanSpy to allow DAHDIChan to become a wrapper for ChanSpy.
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_channelredirect.c 183639 
>   /trunk/apps/app_chanspy.c 183639 
>   /trunk/apps/app_dahdiscan.c 183639 
>   /trunk/apps/app_directed_pickup.c 183639 
>   /trunk/apps/app_minivm.c 183639 
>   /trunk/apps/app_mixmonitor.c 183639 
>   /trunk/apps/app_senddtmf.c 183639 
>   /trunk/apps/app_softhangup.c 183639 
>   /trunk/apps/app_voicemail.c 183639 
>   /trunk/build_tools/cflags.xml 183639 
>   /trunk/channels/chan_agent.c 183639 
>   /trunk/channels/chan_bridge.c 183639 
>   /trunk/channels/chan_dahdi.c 183639 
>   /trunk/channels/chan_gtalk.c 183639 
>   /trunk/channels/chan_iax2.c 183639 
>   /trunk/channels/chan_local.c 183639 
>   /trunk/channels/chan_mgcp.c 183639 
>   /trunk/channels/chan_sip.c 183639 
>   /trunk/channels/chan_unistim.c 183639 
>   /trunk/funcs/func_channel.c 183639 
>   /trunk/funcs/func_global.c 183639 
>   /trunk/funcs/func_logic.c 183639 
>   /trunk/funcs/func_odbc.c 183639 
>   /trunk/include/asterisk/autochan.h PRE-CREATION 
>   /trunk/include/asterisk/channel.h 183639 
>   /trunk/include/asterisk/lock.h 183639 
>   /trunk/main/Makefile 183639 
>   /trunk/main/autochan.c PRE-CREATION 
>   /trunk/main/channel.c 183639 
>   /trunk/main/cli.c 183639 
>   /trunk/main/devicestate.c 183639 
>   /trunk/main/features.c 183639 
>   /trunk/main/logger.c 183639 
>   /trunk/main/manager.c 183639 
>   /trunk/main/pbx.c 183639 
>   /trunk/res/res_agi.c 183639 
>   /trunk/res/res_clioriginate.c 183639 
>   /trunk/res/res_monitor.c 183639 
>   /trunk/res/snmp/agent.c 183639 
> 
> Diff: http://reviewboard.digium.com/r/203/diff
> 
> 
> Testing
> -------
> 
> I've done some basic testing making calls, transfers, etc., and I know Mark has done some, too.  However, this is one of those patches that seems like you can never test it enough.
> 
> Basically, any usage of Asterisk is useful testing for this.
> 
> 
> Thanks,
> 
> Russell
> 
>

More information about the asterisk-dev mailing list