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

Russell Bryant russell at digium.com
Sat Mar 21 12:26:16 CDT 2009 


> On 2009-03-20 14:22:20, Mark Michelson wrote:
> >

Great review, Mark.  Thanks!


> On 2009-03-20 14:22:20, Mark Michelson wrote:
> > /trunk/apps/app_directed_pickup.c, lines 150-152
> > <http://reviewboard.digium.com/r/203/diff/3/?file=3402#file3402line150>
> >
> >     Since you are using an iterator which searches by name anyway, I don't see the purpose of this string comparison here.
> >     
> >     (Note: This comment will not hold quite as true if you switch this function to use ast_channel_callback instead of the iterator).

You're right.  It was unnecessarily left over from the original code.  In any case, I'm changing it all to an ast_channel_callback().


> On 2009-03-20 14:22:20, Mark Michelson wrote:
> > /trunk/apps/app_senddtmf.c, lines 105-108
> > <http://reviewboard.digium.com/r/203/diff/3/?file=3405#file3405line105>
> >
> >     This error message isn't exactly accurate. It just means that the channel specified couldn't be found.
> >     
> >     If you want to check for whether the channel was specified, you should use ast_strlen_zero(channel).
> >     
> >     (I know this isn't new behavior you introduced. I just can't help pointing out inaccuracies :))

Heh, good point.  I'll fix it.


> On 2009-03-20 14:22:20, Mark Michelson wrote:
> > /trunk/apps/app_softhangup.c, lines 105-107
> > <http://reviewboard.digium.com/r/203/diff/3/?file=3406#file3406line105>
> >
> >     I may get flamed for this, but why do we have CAPI-specific code in our apps when we don't package chan_capi?

I have absolutely no idea.  We'd have to "svn blame" it.  I suspect someone asked us to put it in and we figured it didn't really hurt.


> On 2009-03-20 14:22:20, Mark Michelson wrote:
> > /trunk/apps/app_directed_pickup.c, lines 125-160
> > <http://reviewboard.digium.com/r/203/diff/3/?file=3402#file3402line125>
> >
> >     This function seems like it would benefit from using ast_channel_callback instead of an iterator.

This is a good idea.  I don't think I had added ast_channel_callback() when I did this part.  I'll give it a whirl.


> On 2009-03-20 14:22:20, Mark Michelson wrote:
> > /trunk/include/asterisk/channel.h, lines 392-394
> > <http://reviewboard.digium.com/r/203/diff/3/?file=3420#file3420line392>
> >
> >     There are a few places where the rule about using ast_change_name to change a name is violated (in fact, it appears that the function is completely unused at this point).
> >     
> >     1. In channel.c, ast_do_masquerade renames channels directly a few times.
> >     
> >     2. In chan_dahdi.c, pri_fixup_principle renames a channel directly.
> >     
> >     The last two are places where the channel is renamed, but rather than changing the way they are renamed, the renaming should probably just be removed/reworked.
> >     
> >     3. In chan_mgcp.c, mgcp_new "renames" the channel to exactly the same thing that ast_channel_alloc named it. This won't cause hashing problems, but it is really dumb.
> >     
> >     4. In chan_unistim.c, unistim_new appears to rename the channel to something different than what ast_channel_alloc named the channel. Specifically, the portion after the '-' in the channel name appears to be different. This seems like it would cause some bad hashing errors.

Thanks for doing this investigation work.  You are absolutely correct that these bugs would cause hashing errors.  I'll work on fixing these up.


> On 2009-03-20 14:22:20, Mark Michelson wrote:
> > /trunk/include/asterisk/channel.h, line 1632
> > <http://reviewboard.digium.com/r/203/diff/3/?file=3420#file3420line1632>
> >
> >     s/for the now/for now/

fixed.


> On 2009-03-20 14:22:20, Mark Michelson wrote:
> > /trunk/main/channel.c, lines 1330-1336
> > <http://reviewboard.digium.com/r/203/diff/3/?file=3424#file3424line1330>
> >
> >     I think this would read a bit more clearly if you moved the ast_channel_unlock(chan) to above the break. That way the lock and matching unlock are at the same scope/indentation level.

done!


> On 2009-03-20 14:22:20, Mark Michelson wrote:
> > /trunk/main/cli.c, line 839
> > <http://reviewboard.digium.com/r/203/diff/3/?file=3425#file3425line839>
> >
> >     The c = ast_channel_iterator_next(iter) statement should probably be the loop-continuing condition instead of a loop maintenance operation.
> >     
> >     In other words, move that to in between the semicolons in the for loop. In fact, upon looking at this more closely, I think this for loop won't even work the way it is written since c is NULL the first time the loop is reached.

Yeah.  :-(  Fixed!


> On 2009-03-20 14:22:20, Mark Michelson wrote:
> > /trunk/main/cli.c, lines 1269-1271
> > <http://reviewboard.digium.com/r/203/diff/3/?file=3425#file3425line1269>
> >
> >     Remove the ! here.

Fixed!


> On 2009-03-20 14:22:20, Mark Michelson wrote:
> > /trunk/res/snmp/agent.c, lines 250-259
> > <http://reviewboard.digium.com/r/203/diff/3/?file=3434#file3434line250>
> >
> >     I'm not sure whether i is a 0-based or 1-based index, but if it is 1-based, then there is an off-by-one error here and the last channel that the iterator reaches will not be analyzed.
> >     
> >     However, if reaching the 5th channel means that i starts off with a value of 4, then this loop is just fine.

I went back and looked at the code that is in trunk, and I believe that i is a 0-based index.  Here it is for reference:

    for (chan = ast_channel_walk_locked(NULL);
         chan && i;
         chan = ast_channel_walk_locked(chan), i--)
        ast_channel_unlock(chan);


- Russell


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


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