[asterisk-dev] [Code Review] Fix extension state callback references in chan_sip.

[schmidts]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1635/#review5043
-----------------------------------------------------------

Ship it!


I was going through all the changes and it looks fine to me. also the destroy order of dialog_unlink_all is now much clearer and i understand why this could lead to problems when the subscribe obj will be destroyed while its still linked to the dialog.

Making the locking order more simple is allways a good idea!

the two points i notice are just very short and easy questions.


/branches/1.8/main/pbx.c
<https://reviewboard.asterisk.org/r/1635/#comment9301>

    imho it would be a good idea to add a flag here if this is a global callback or not and also have unique ids also on global callbacks.
    But i think this would have too much impact on the code for 1.8 ;)



/branches/1.8/main/pbx.c
<https://reviewboard.asterisk.org/r/1635/#comment9302>

    maybe changing this to static const unsigned hash_extenhint_size would also be ok?


- schmidts


On Dec. 20, 2011, 4 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1635/
> -----------------------------------------------------------
> 
> (Updated Dec. 20, 2011, 4 p.m.)
> 
> 
> Review request for Asterisk Developers and Matt Jordan.
> 
> 
> Summary
> -------
> 
> Fix extension state callback references in chan_sip.
> 
> Chan_sip gives a dialog reference to the extension state callback and
> assumes that when ast_extension_state_del() returns, the callback cannot
> happen anymore.  Chan_sip then reduces the dialog reference count
> associated with the callback.  Recent changes (ASTERISK-17760) have
> resulted in the potential for the callback to happen after
> ast_extension_state_del() has returned.  For chan_sip, this could be very
> bad because the dialog pointer could have already been destroyed.
> 
> * Added ast_extension_state_add_destroy() so chan_sip can account for the
> sip_pvt reference given to the extension state callback when the extension
> state callback is deleted.
> 
> * Fix pbx.c awkward statecbs handling in ast_extension_state_add_destroy()
> and handle_statechange() now that the struct ast_state_cb has a destructor
> to call.
> 
> * Ensure that ast_extension_state_add_destroy() will never return -1 or 0
> for a successful registration.
> 
> * Fixed pbx.c statecbs_cmp() to compare the correct information.  The
> passed in value to compare is a change_cb function pointer not an object
> pointer.
> 
> * Make pbx.c ast_merge_contexts_and_delete() not perform callbacks with
> AST_EXTENSION_REMOVED with locks held.  Chan_sip is notorious for
> deadlocking when those locks are held during the callback.
> 
> * Removed unused lock declaration for the pbx.c store_hints list.
> 
> 
> This addresses bug ASTERISK-18844.
>     https://issues.asterisk.org/jira/browse/ASTERISK-18844
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_sip.c 348734 
>   /branches/1.8/include/asterisk/pbx.h 348734 
>   /branches/1.8/main/pbx.c 348734 
> 
> Diff: https://reviewboard.asterisk.org/r/1635/diff
> 
> 
> Testing
> -------
> 
> It compiles and basic call testing only.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20111221/89338df4/attachment.htm>