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

[Olle E. Johansson]

9 apr 2009 kl. 20.16 skrev Russell Bryant:

> 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.
>
Without looking into any of the two solutions, I just wanted to make
sure that you are aware that in manager 1.1 we have a module management
manager action.

/O