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

[Russell Bryant]

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

Ship it!


All of my remaining comments are really minor.  Nice work, Jeff!


/branches/1.4/apps/app_meetme.c
<https://reviewboard.asterisk.org/r/746/#comment5226>

    red death mark



/branches/1.4/apps/app_meetme.c
<https://reviewboard.asterisk.org/r/746/#comment5227>

    It's possible for this to be NULL, so that should be checked.



/branches/1.4/apps/app_meetme.c
<https://reviewboard.asterisk.org/r/746/#comment5228>

    add braces while you're at it



/branches/1.4/apps/app_meetme.c
<https://reviewboard.asterisk.org/r/746/#comment5229>

    You could move the ao2_ref() down to before the break to avoid having to do it in 2 places.


- Russell


On 2010-07-01 12:23:49, Jeff Peeler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/746/
> -----------------------------------------------------------
> 
> (Updated 2010-07-01 12:23:49)
> 
> 
> 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 273395 
> 
> 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
> 
>