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

Russell Bryant russell at digium.com
Fri Mar 20 09:44:52 CDT 2009 


> On 2009-03-20 09:17:57, Mark Michelson wrote:
> > /trunk/apps/app_directed_pickup.c, lines 208-210
> > <http://reviewboard.digium.com/r/203/diff/1/?file=3299#file3299line208>
> >
> >     Missing an ast_channel_unlock here.

This is intentional.  We leave target locked before calling pickup_do() a few lines later.


> On 2009-03-20 09:17:57, Mark Michelson wrote:
> > /trunk/funcs/func_global.c, lines 154-157
> > <http://reviewboard.digium.com/r/203/diff/1/?file=3312#file3312line154>
> >
> >     Since locking the channel is now a separate operation from finding the channel, you can actually get rid of this else block and just call ast_channel_lock(chan) after the if block.

Done!


> On 2009-03-20 09:17:57, Mark Michelson wrote:
> > /trunk/funcs/func_global.c, lines 213-216
> > <http://reviewboard.digium.com/r/203/diff/1/?file=3312#file3312line213>
> >
> >     Same thing here, too.

Done!


> On 2009-03-20 09:17:57, Mark Michelson wrote:
> > /trunk/main/channel.c, lines 130-131
> > <http://reviewboard.digium.com/r/203/diff/1/?file=3320#file3320line130>
> >
> >     Really?!

lol ... sure, why not?  I mean, it's pretty arbitrary, but on the high end of Asterisk installs, we're talking about the number of channels being in the 10,000 range. 


> On 2009-03-20 09:17:57, Mark Michelson wrote:
> > /trunk/main/channel.c, line 5506
> > <http://reviewboard.digium.com/r/203/diff/1/?file=3320#file3320line5506>
> >
> >     Yay for getting rid of this crap. Good riddance!
> >     
> >     However, it appears that the DEBUG_CHANNEL_LOCKS options is still selectable in menuselect. It should be removed.

Removed!


> On 2009-03-20 09:17:57, Mark Michelson wrote:
> > /trunk/channels/chan_local.c, lines 743-745
> > <http://reviewboard.digium.com/r/203/diff/1/?file=3309#file3309line743>
> >
> >     I realize that what you've done is just convert what was previously here, but this section dealing with tmp2 is actually not necessary if you think about it.

You're right.  :-)  Removed!


> On 2009-03-20 09:17:57, Mark Michelson wrote:
> > /trunk/include/asterisk/channel.h, line 1886
> > <http://reviewboard.digium.com/r/203/diff/1/?file=3316#file3316line1886>
> >
> >     Trailing whitespace. I'll let review board's red highlighting point you to the other instances.

I have removed all whitespace errors in code changed in this patch.  I may have gotten carried away in a few places and fixed a few extras ...


- Russell


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


On 2009-03-20 07:35:51, Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/203/
> -----------------------------------------------------------
> 
> (Updated 2009-03-20 07:35:51)
> 
> 
> 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 183544 
>   /trunk/apps/app_chanspy.c 183544 
>   /trunk/apps/app_dahdiscan.c 183544 
>   /trunk/apps/app_directed_pickup.c 183544 
>   /trunk/apps/app_minivm.c 183544 
>   /trunk/apps/app_mixmonitor.c 183544 
>   /trunk/apps/app_senddtmf.c 183544 
>   /trunk/apps/app_softhangup.c 183544 
>   /trunk/apps/app_voicemail.c 183544 
>   /trunk/channels/chan_agent.c 183544 
>   /trunk/channels/chan_bridge.c 183544 
>   /trunk/channels/chan_gtalk.c 183544 
>   /trunk/channels/chan_iax2.c 183544 
>   /trunk/channels/chan_local.c 183544 
>   /trunk/channels/chan_sip.c 183544 
>   /trunk/funcs/func_channel.c 183544 
>   /trunk/funcs/func_global.c 183544 
>   /trunk/funcs/func_logic.c 183544 
>   /trunk/funcs/func_odbc.c 183544 
>   /trunk/include/asterisk/autochan.h PRE-CREATION 
>   /trunk/include/asterisk/channel.h 183544 
>   /trunk/include/asterisk/lock.h 183544 
>   /trunk/main/Makefile 183544 
>   /trunk/main/autochan.c PRE-CREATION 
>   /trunk/main/channel.c 183544 
>   /trunk/main/cli.c 183544 
>   /trunk/main/devicestate.c 183544 
>   /trunk/main/features.c 183544 
>   /trunk/main/logger.c 183544 
>   /trunk/main/manager.c 183544 
>   /trunk/main/pbx.c 183544 
>   /trunk/res/res_agi.c 183544 
>   /trunk/res/res_clioriginate.c 183544 
>   /trunk/res/res_monitor.c 183544 
>   /trunk/res/snmp/agent.c 183544 
> 
> 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