[asterisk-dev] channel.c locking and other

[Damien Wedhorn]

I'm tidying up chan_skinny's chan->tech_pvt handling which sort of meant 
screwing with the locking. To this end, I've removed a bunch of locks 
from chan_skinny so I basically only have a device lock and use the chan 
lock in combination with the device lock when required. Anyway, to avoid 
deadlocking, I basically make sure I have the chan lock before calling 
any channel.c functions.

I thought this a bit overzealous and so had a look through channel.c and 
ended up with a bunch of wtf's. Admitting that I really haven't looked 
at channel.c in much detail previously, I noted the following:

1 ast_queue_hangup basically goes "!trylock(set softhangup, unlock), 
lock, add queue frame, unlock". Why not just lock the chan to begin with 
rather than unlocking it to immediately relock it?

2 ast_queue_frame_head's only use appears to be to fix AST_LIST_ADD_HEAD 
in ast_answer. If AST_LIST_ADD_TAIL was used, the function and 
associated code for ast_queue_frame_head could be removed.

3 chan->_softhangup seems to be set locked and unlocked. It's a bit mask 
and some comparisons are ==, it is sometimes specifically assigned 
values without checking whether something is already there. Overall 
there doesn't appear to be enough consistency to ensure that set values 
aren't overwritten.

4 why does __ast_queue_frame use the channel lock. Couldn't we queue and 
dequeue based on a list lock? This is the biggest for me. If we used a 
list lock or similar, I could remove most (if not all) chan locking in 
chan_skinny. May apply to other channels as well. However, I'm not 
familiar enough with all the ins and outs of channel.c (and how it 
relates to all the other code) to know if this is feasible.

Just a general comment. Some doxygen comments for channel.c indicate 
that chan_locking is not required for certain functions. It may be worth 
including whether or not that function locks the chan, because if it 
does and a thread holds another lock (not a chan lock), the chan may 
need to be locked to avoid a potential deadlock. Also, some functions 
have comments that they do not lock the chan when they actually do.

I'm aware that a lot of this is probably just leftovers from previous 
code, but I would especially like some feedback on the feasibility/issue 
of using a list lock rather than the chan lock for frame 
queueing/dequeueing.

Regards

Damien Wedhorn