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

[rmudgett]

> On July 21, 2011, 11:24 a.m., David Vossel wrote:
> > /branches/1.8/main/pbx.c, lines 4286-4289
> > <https://reviewboard.asterisk.org/r/1313/diff/1-2/?file=17415#file17415line4286>
> >
> >     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.

I personally don't care for the combined format.  It makes long line breaks awkward whereas with separate statements there are plenty of convenient locations.  Also the two forms should be identical to the compiler as far as code generation is concerned.


> On July 21, 2011, 11:24 a.m., David Vossel wrote:
> > /branches/1.8/main/pbx.c, lines 7331-7355
> > <https://reviewboard.asterisk.org/r/1313/diff/1-2/?file=17415#file17415line7331>
> >
> >     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.

Modified your suggestion somewhat to make the code clearer.


> On July 21, 2011, 11:24 a.m., David Vossel wrote:
> > /branches/1.8/main/pbx.c, lines 10448-10466
> > <https://reviewboard.asterisk.org/r/1313/diff/1-2/?file=17415#file17415line10448>
> >
> >     Is it possible for a hint to be in the hints container without a extension?

I don't think so.  This code is defensive.  The only external difference is if the exten doesn't exist or is empty the function returns 0 instead of -1.  Returning a negative value for the hash function is benign to the caller and is equivalent to 0.


> On July 21, 2011, 11:24 a.m., David Vossel wrote:
> > /branches/1.8/main/pbx.c, lines 4485-4494
> > <https://reviewboard.asterisk.org/r/1313/diff/1-2/?file=17415#file17415line4485>
> >
> >     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) {
> >     ...
> >     }
> 
> David Vossel wrote:
>     I was wrong, forget this one.

I did this change because I read the "if (!id && !callback)" as "if (!id || !callback)" since it looks like normal parameter sanity check code.


> On July 21, 2011, 11:24 a.m., David Vossel wrote:
> > /branches/1.8/main/pbx.c, lines 4668-4689
> > <https://reviewboard.asterisk.org/r/1313/diff/1-2/?file=17415#file17415line4668>
> >
> >     nice catch!

I don't think this was an actual problem, but it was wrong.  The function is only called by one location to replace the hint with an updated version.  The exten string is not expected to be different in this case.


- rmudgett


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


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/27d81893/attachment-0001.htm>