[asterisk-dev] tilghman: branch 1.4 r187428 - in /branches/1.4: include/asterisk/ main/

[Tilghman Lesher]

On Thursday 09 April 2009 13:16:32 Russell Bryant wrote:
> SVN commits to the Digium repositories wrote:
> > Author: tilghman
> > Date: Thu Apr  9 13:08:20 2009
> > New Revision: 187428
> >
> > URL: http://svn.digium.com/svn-view/asterisk?view=rev&rev=187428
> > Log:
> > Race condition between ast_cli_command() and 'module unload' could cause
> > a deadlock. Add lock timeouts to avoid this potential deadlock.
> > (closes issue #14705)
> >  Reported by: jamessan
> >  Patches:
> >        20090320__bug14705.diff.txt uploaded by tilghman (license 14)
> >  Tested by: jamessan
> >
> > Modified:
> >     branches/1.4/include/asterisk/lock.h
> >     branches/1.4/main/manager.c
> >
> >
> > Modified: branches/1.4/main/manager.c
> > URL:
> > http://svn.digium.com/svn-view/asterisk/branches/1.4/main/manager.c?view=
> >diff&rev=187428&r1=187427&r2=187428
> > =========================================================================
> >===== --- branches/1.4/main/manager.c (original)
> > +++ branches/1.4/main/manager.c Thu Apr  9 13:08:20 2009
> > @@ -2608,8 +2608,12 @@
> >  int ast_manager_unregister(char *action)
> >  {
> >  	struct manager_action *cur, *prev;
> > -
> > -	ast_rwlock_wrlock(&actionlock);
> > +	struct timespec tv = { 5, };
> > +
> > +	if (ast_rwlock_timedwrlock(&actionlock, &tv)) {
> > +		ast_log(LOG_ERROR, "Could not obtain lock on manager list\n");
> > +		return -1;
> > +	}
> >  	cur = prev = first_action;
> >  	while (cur) {
> >  		if (!strcasecmp(action, cur->action)) {
>
> I'm actually more in favor of the originally proposed patch by jamesson
> on issue 14705.
>
> As it stands, we now have two things in the code dealing with this
> potential deadlock.  We have the CLI command blacklist, which jamesson
> simply proposed adding "module reload" to, and now we have this timed
> lock stuff.
>
> This timed lock handling will result in less deterministic behavior.  In
> some cases, module CLI commands will work from the AMI, and in other
> cases they won't.  I would much rather just make it clear that it is not
> supported at all by sticking with the blacklist approach.

The only module command that will occasionally not work is the 'module unload'
command.  The 'module reload' command will always work (with this patch), and
if you ask users, I suspect that a great many of them will resent not being
able to perform reloads via AMI, whereas the likelihood of them resenting not
being able to unload a module is much lower.  That's why I pursued this
approach.

-- 
Tilghman