[asterisk-dev] [Code Review] Make user removals and traversals thread safe in meetme

[Jeff Peeler]

> On 2010-06-25 13:57:08, Russell Bryant wrote:
> > /branches/1.4/apps/app_meetme.c, lines 1080-1088
> > <https://reviewboard.asterisk.org/r/746/diff/1/?file=11121#file11121line1080>
> >
> >     You have changed the behavior here.  Previously, "1<tab>" would would output "1", "10", "11", "12", etc.  Now, I think it's only going to output "1".

Ok yes, I really messed this part up. Will fix.


> On 2010-06-25 13:57:08, Russell Bryant wrote:
> > /branches/1.4/apps/app_meetme.c, line 1078
> > <https://reviewboard.asterisk.org/r/746/diff/1/?file=11121#file11121line1078>
> >
> >     It would be good to do some validation here by using sscanf() and making sure it returns 1 instead of atoi().

This is going away.


> On 2010-06-25 13:57:08, Russell Bryant wrote:
> > /branches/1.4/apps/app_meetme.c, lines 3074-3075
> > <https://reviewboard.asterisk.org/r/746/diff/1/?file=11121#file11121line3074>
> >
> >     find_user() now returns a reference to a user, and it looks like there are a number of places where the returned reference will be leaked.

Taken care of with the other args.user check at the end of the function.


> On 2010-06-25 13:57:08, Russell Bryant wrote:
> > /branches/1.4/apps/app_meetme.c, lines 3161-3164
> > <https://reviewboard.asterisk.org/r/746/diff/1/?file=11121#file11121line3161>
> >
> >     All of the ao2_iterator usages in this function are actually good candidates for ao2_callback implementations, instead.

What is the advantage of using ao2_callback here since all the iterator variables are already present due to being necessary for other cases?


- Jeff


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


On 2010-06-25 11:52:41, Jeff Peeler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/746/
> -----------------------------------------------------------
> 
> (Updated 2010-06-25 11:52:41)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> The race conditions present in meetme involves the user list where a lack of locking has the potential for a user to be removed during a traversal or as in the case of the reporter after checking if the list is empty. Adding locking would mostly solve the problem, but there were some cases that abuse of the list lock was required to protect operations on the user (such as after using AST_LIST_LAST). Therefore I went the slightly more invasive route of ao2-ifying the users to make everything completely safe.
> 
> 
> This addresses bug 17390.
>     https://issues.asterisk.org/view.php?id=17390
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/apps/app_meetme.c 272526 
> 
> Diff: https://reviewboard.asterisk.org/r/746/diff
> 
> 
> Testing
> -------
> 
> Tested joining, parting conference and verified assigned user number was as expected. Randomly tested ejecting and kicking all.
> 
> 
> Thanks,
> 
> Jeff
> 
>