[asterisk-dev] chan->_softhangup

[Damien Wedhorn]

Tilghman Lesher wrote:
> On Sunday 21 February 2010 04:18:16 Damien Wedhorn wrote:
>   
>> Tilghman Lesher wrote:
>>     
>>> It's basically used as a general purpose inter-thread signal.  Whether
>>> you call it softhangup or not, I don't think that the semantics are
>>> terribly difficult, although the fact that all the possible causes are
>>> gathered together in a single bitwise field is nice.
>>>       
>> That's the fundamental problem. It is both a general purpose bitfield
>> and a specific purpose variable. If any bit is set and there is an
>> active bridge, the bridge will be torn down unless the bits are all
>> reset before the lock is released, irrespective of what the intent of
>> the specific bit is. My initial mail probably wasn't that clear.
>>     
>
> The intent of every bit is indeed to cause the bridge to get torn down,
> applications to return, etc.  It's a general purpose interrupt, with lots of
> individual causes.  That's part of what makes it such a fine solution.  I
> wonder if you looked at the solution, saw the many causes, and assumed
> it was a poor implementation, rather than a brilliant one.
>   
I'm happy to admit that maybe it's a brilliant solution, but I can't see
it and need someone to show it's brilliance. My main issue was that I
saw the many causes, but saw only two places using the bitwise results.
The first in pbx.c, which gets reset in the code and nothing gets torn
down (it's used for timeouts before moving to the next priority).

The other place it gets used in a bitwise sense is channel.c which tests
for AST_SOFTHANGUP_UNBRIDGE, in which case it resets all _softhangup
bits to zero and doesn't tear down anything. It also appears this is
only set from within chanspy and mixmonitor.

The bit that really makes me think this is a poor implementation is that
the only use of the bitwise data (ie do not tear down anything), is
completely at odds with the general purpose of tearing the bridge down.

The other issue is the unreliability in setting the bitfield.
ast_queue_hangup does a trylock before toggling AST_SOFTHANGUP_DEV,
irrespective of what value it held before. Any code that expects the
AST_SOFTHANGUP_DEV bit to be set after calling ast_queue_hangup is
asking for trouble. The only reliable result of ast_queue_hangup is that
a hangup frame is at the end of the queue.

All through the code base a single bit is toggled without any reference
as to what is already there. That is, it should be treated as a randomly
set bit. If there is any testing, it's if(_softhangup) which tests all
of the bits. So in some cases, a cause will not be toggled because
another bit is already set. Doesn't sound like a normal bitfield use to me.

I'm happy to be proven wrong, because atomicising _softhangup is
arguably easier than removing it, and wherever the atomic use breaks
asterisk it is because there was already a bug. Removing is
significantly more difficult as any breaks will be my fault in which
case I'll probably have to fix it.

Damien Wedhorn