[asterisk-dev] mmichelson: trunk r157427 - /trunk/channels/chan_sip.c

[Mark Michelson]

Mark Michelson wrote:
> Russell Bryant wrote:
>> Mark Michelson wrote:
>>> Russell Bryant wrote:
>>>> SVN commits to the Digium repositories wrote:
>>
>>>>> +/*! \brief Protect the callcounters inuse,inringing and the 
>>>>> corresponding flags */
>>>>> +AST_MUTEX_DEFINE_STATIC(callctrlock);
>>>>> +
>>>> What is the logic behind using this new global lock for this 
>>>> purpose? What is wrong with just ensuring that the sip_pvt is locked 
>>>> during these operations?
>>>>
>>> The reason is that we are protecting boht flags that are in the 
>>> sip_pvt structure and more importantly counters that are in the 
>>> sip_peer structure (and in the case of 1.6.0, the sip_user structure, 
>>> too). Because sip_peers and sip_pvts do not necessarily share a 1:1 
>>> relationship, the data in the sip_peer is not sufficiently protected 
>>> by just locking the sip_pvt.
>>
>> Ok, then it seems like the right thing is to have the sip_pvt and the 
>> sip_peer locked, then.
>>
> 
> I thought about doing that, too, but I couldn't really see any added 
> value in adding a lock to the sip_peer structure over the solution that 
> was presented in the provided patch. I suppose that since we are 
> protecting data that is part of the sip_peer structure, it makes sense 
> to add locks to the sip_peer and sip_user structures. I'll go ahead and 
> make these changes.
> 
> Mark Michelson
> 

Ah, I completely missed the fact that sip_peers are ao2 objects, and so they 
already have a lock.

However, this makes things very tricky for implementing in 1.6.0. The reason is 
that in that branch, we still have both sip_user and sip_peer structures. In 
update_call_counter, we save a local pointers which may point to sip_user or 
sip_peer counters. We can't do the same thing with their locks since they are 
part of the ao2 data that we cannot directly access from within chan_sip. I 
think we're stuck with the global lock solution for 1.6.0

Mark Michelson