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

[Russell Bryant]

Luigi Rizzo wrote:
> 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.

Oh, right ... thank you for pointing that out!

> 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.

Will do.

> 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.

Thanks for the feedback.  I will incorporate your suggestions now.

-- 
Russell Bryant
Software Engineer
Digium, Inc.