[asterisk-dev] [Code Review] Deadlocks dealing with dialplan hints during reload.

[David Vossel]

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


I'm still looking through this.  Just updating while I'm taking a break.


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

    This does not matter much, but it is a common pattern within Asterisk to collapse both the allocation of a pointer and the check to make sure it occurred without failure into a single line within an if statement.  If we are just going to allocate something right before checking that allocation, it is probably worth collapsing the allocation down into the if statement... but like I said. It doesn't matter.



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

    Moving this if block seems to make this function look more complicated than it is.
    
    It would probably be best to either leave it the way it was or do something like this that is more self documenting.
    
    if (callback) {
    ...
    } else if (id) {
    ...
    }



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

    nice catch!



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

    This logic shouldn't have to change.  If you want to avoid the case of trying to find a hint with a NULL exten, why not just do
    
    hint = exten ? ao2_find(hints, exten, 0) : NULL;
    
    before the if statement.



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

    Is it possible for a hint to be in the hints container without a extension?


- David


On July 14, 2011, 8:20 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1313/
> -----------------------------------------------------------
> 
> (Updated July 14, 2011, 8:20 p.m.)
> 
> 
> Review request for Asterisk Developers, Russell Bryant, David Vossel, and irroot.
> 
> 
> Summary
> -------
> 
> There are two remaining different deadlocks reported dealing with dialplan
> hints.
> 
> The deadlock in ASTERISK-17666 is caused by invalid locking order in
> ast_remove_hint().  The hints container must be locked before the hint
> object.
> 
> The deadlock in ASTERISK-17760 is caused by a catch-22 situation in
> handle_statechange().  The deadlock is caused by not having the conlock
> before calling the watcher callbacks.  Unfortunately, having that lock
> causes a different deadlock as reported in ASTERISK-16961.
> 
> * Fixed ast_remove_hint() locking order.
> 
> * Made handle_statechange() no longer call the watcher callbacks holding
> any locks that matter.
> 
> * Made hint ao2 destructor do the watcher callbacks for extension
> deactivation to guarantee that they get called.
> 
> * Fixed hint reference leak in ast_add_hint() if the callback container
> constructor failed.
> 
> * Fixed hint reference leak in complete_core_show_hint() for every hint it
> found for CLI tab completion.
> 
> * Adjusted locking in ast_merge_contexts_and_delete() for safety.
> 
> * Added context_merge_lock to prevent ast_merge_contexts_and_delete() and
> handle_statechange() from interfering with each other.
> 
> * Fixed ast_change_hint() not taking into account that the extension is
> used for the hash key.
> 
> 
> This addresses bugs ASTERISK-17666 and ASTERISK-17760.
>     https://issues.asterisk.org/jira/browse/ASTERISK-17666
>     https://issues.asterisk.org/jira/browse/ASTERISK-17760
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/main/pbx.c 328355 
> 
> Diff: https://reviewboard.asterisk.org/r/1313/diff
> 
> 
> Testing
> -------
> 
> * Did some calls that changed the state of some hints.
> * Reloaded the dialplan.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110721/d4532617/attachment-0001.htm>