[asterisk-dev] [asterisk-commits] russell: branch russell/chan_refcount r82328 - in /team/russell/chan_refcount...

[Luigi Rizzo]

On Thu, Sep 13, 2007 at 04:30:50PM -0000, SVN commits to the Asterisk project wrote:
> Author: russell
> Date: Thu Sep 13 11:30:50 2007
> New Revision: 82328
> 
> URL: http://svn.digium.com/view/asterisk?view=rev&rev=82328
> Log:
> * constify some args to manager functions
> * add two new functions ao2_lock_both() and ao2_unlock_both()
>   - These functions are intended to be used when two objects must be locked
>     at the same time, and thus deadlocks must be avoided.  However, instead of
> 	using an expensive trylock/unlock/lock/sleep/trylock loop, it just locks
> 	the object with a lower pointer address first to ensure that the locking
> 	is always done in the same order.


The way you implement it, ao2_lock_both() won't save
you if the thread already happens to hold one of the
locks (which is a constant danger with recursive locks).
The implementation around a trylock/sleep was at least safe
from that point of view.

Secondly, i would suggest to implement ao2_lock_both() in terms
of ao2_lock() and ao2_trylock(), and don't play with the internals
of the ao2 object itself.

As for ao2_unlock_both(), as you clearly mention, it is unnecessary,
and i'd rather remove it altogether: there is basically no advantage
in terms of source/binary code size or runtime performance, and
there are many reasons against, as this new function is obfuscating
the source (it may suggests that it does something special while
it doesn't), and in general each additional method makes a "class"
more complex to use, maintain and possibly replace with a newer
implementation.

It may be resaonable to define, locally to a file, a wrapper around
a common code pattern, but that too should be done only when the
pattern is used often enough to be worth the replacement.

	cheers
	luigi