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

[Mark Michelson]

Mark Michelson wrote:
> 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
> 

And checking in 1.6.0 more thoroughly, it seems like sip_peers are *not* ao2 
objects in that branch, so I can just add a new lock. Geez, this is complicated :)

Mark Michelson