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

[schmidts]

> On Dec. 21, 2011, 5:15 a.m., schmidts wrote:
> > /branches/1.8/main/pbx.c, lines 921-922
> > <https://reviewboard.asterisk.org/r/1635/diff/1/?file=22283#file22283line921>
> >
> >     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 ;)
> 
> rmudgett wrote:
>     The id value being zero is the flag indicating that it is a global callback.  Global callbacks distinguish themselves from each other by the callback function pointer.

my intention about this was, that you could use a normal hash for the global callbacks not only a link list. Maybe this is allready possible by hashing the callback function but as i have written before this would have too much impact on 1.8.


> On Dec. 21, 2011, 5:15 a.m., schmidts wrote:
> > /branches/1.8/main/pbx.c, lines 957-959
> > <https://reviewboard.asterisk.org/r/1635/diff/1/?file=22283#file22283line957>
> >
> >     maybe changing this to static const unsigned hash_extenhint_size would also be ok?
> 
> rmudgett wrote:
>     C code should not be using C++ idioms.  In this case, the code has a slightly different meaning in the linkage.  C actually declares a variable with the initialization value.  C++ does not necesisarily declare a variable but a compile time constant with behavior similar to a define constant.

thanks for pointing this out. i just wasnt sure about it ;)


- schmidts


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


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/b063882d/attachment.htm>