[asterisk-dev] chan->_softhangup

[Damien Wedhorn]

Damien Wedhorn wrote:
> 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.
>   
Sorry all, this previous para is wrong. The bitfield's are |'d not &'d.
Must connect mailserver to coffee machine so that no emails are sent
until after first coffee of the morning.
> 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
>
>