[asterisk-dev] sig_ss7 softhangup

[Richard Mudgett]

> Hi,
>  Revision 357721 have changed sig_ss7 and
>    	linkset->pvts[i]->owner->_softhangup |= AST_SOFTHANGUP_DEV;
> became
> 	ast_channel_softhangup_internal_flag_add(linkset->pvts[i]->owner,
> AST_SOFTHANGUP_DEV);
> 
> later in Revision 358307 again
> ast_channel_softhangup_internal_flag_add()
> is called, but in Asterisk 10 the same change uses:
> 	chan->_softhangup |= AST_SOFTHANGUP_DEV;
> while the rest of the code in sig_ss7 uses:
> 	ast_softhangup_nolock(p->owner, AST_SOFTHANGUP_DEV);
> 
> the result is that we have many ways of doing almost the same and
> (the
> problem for me actualy) the patch from review 1676 is just no longer
> compatible between Asterisk 10 and trunk. It would be good to have
> the
> patch compatible until the code is submitted in order to have it
> available
> for testing from more users.

New features and architecture enhancements are done to trunk.  That is
its purpose.  Patches are not guaranteed to be applicable between revisions
of a release series much less between branches.  Trunk just went through an
ast_channel opaquification phase to remove layout dependencies to the
ast_channel struct throughout the codebase.

> 
> Is there a reason not to use ast_softhangup_nolock() in all places,
> which
> in addition queues a frame to the channel and interrupts any
> blockers?

The use of ast_softhangup_nolock() requires channel locking
considerations because ast_softhangup_nolock() calls ast_queue_frame()
internally which locks the channel.  The nolock suffix is intended to
imply that you already have the channel lock held before calling.

The _softhangup channel value is not supposed to be set directly because
ast_softhangup() needs to potentially wake up the media handling thread
early.  The reason _softhangup is still set directly is because there
were so many channel drivers that did so before ast_softhangup() was
created.  The ast_softhangup() call also has different assumptions that
need to be considered before converting code that directly sets the
_softhangup flags to calling ast_softhangup/ast_softhangup_nolock.

The new places in sig_ss7_indicate() that are setting the _softhangup
flags should be changed to ast_softhangup_nolock() since the channel
lock is already held.  I was using the code in sig_pri.c as a guide
to how to handle the control frames.

The only other place where the _softhangup flags are set directly in
sig_ss7.c is ss7_hangup_cics().  To convert it to using the
ast_softhangup_nolock() requires locking the owner with
sig_ss7_lock_owner() to acquire the channel lock without deadlock.

I will make both of these changes.

> 
> P.S.
>  The comment for 358307 says "Made SS7 hangup a call immediately",
>  but
> _immedeatly_ will be if ast_softhangup_nolock is called actually.

Immediately is relative.  Just setting the _softhangup flag is usually
sufficient to hangup the channel "immediately" because the media handling
thread runs frequently.

Richard