[OpenSER-Devel] discussion: issues with local_route

[Daniel-Constantin Mierla]

Hello,

On 06/20/08 13:49, Bogdan-Andrei Iancu wrote:
> Hi Daniel,
>
> To summarize :
>
> 1) script flag - indeed there is an issue there and fortunately the 
> fix trivial - please open a bug report just not to forget about it
>
> 2) branch and branch_flags related function - these are not intended 
> to be used in local  route as it makes no sense. As, said, the core 
> does not allow any possibility to disable the core functions, so do 
> not use it. If you use it (against the description), you may loose 
> some info. But again, it is not a bug or missing, it is just about 
> using inappropriate functions in a route.
>
> 3) time PVs - as time as the PVs are bind to the message (by design), 
> it is normal to be refreshed when the processed message changes.
>
> I tried to addressed all your remarks that have a technical base. If I 
> missed some, please list them along arguments.
I started to believe you don't understand exactly what happens. You now 
blame functions and functionalities that are there for ages, instead of 
allowing time for a proper analysis, design and integration of the 
local_route. You claimed that local route is a small patch with no 
impact on architecture or other code, perhaps it is no longer valid.

If you do not log branches, branch flags in acc or xlog, does not mean 
nobody else is doing. It is poor excuse to go with such motivation - the 
application must be coherent and consistent.

The time PVs are there for 5 years or so (no matter was $ or %), and the 
config worked the same - when I printed via xlog, everywhere in my main 
route block, was the same value for the same sip message. Now is 
different and you say it is fault of that functions?!?! People have 
monitoring, statistics or other tools that rely on this functionality. 
You should let the developers that implemented the functions to argument 
what was the designed behavior, don't recall that for time PVs were you.

So, please, avoid argumentations with "inappropriate usage" or "not 
designed to be used there (but allowed)", it is just your opinion, 
besides that quality software and projects don't go out with such stuff 
in major releases.

To go further and complete the 1.4 release, the solution I see is to 
enclose local_route in defines in disable it by default. Who wants to 
use it, can recompile and reinstall from sources. Fortunately, 1.5 will 
include it with the right design.

Cheers,
Daniel


>
> Regards,
> Bogdan
>
>
> Daniel-Constantin Mierla wrote:
>>
>>>
>>>> - setsflag() broken
>>>> - resetsflag() broken
>>>> - issflagset() broken
>>>>   
>>> why are broken? the script flags are never reset before or after any 
>>> type of route.
>>> So, the script flags behave in local route exactly as in any other 
>>> type of route.
>>
>> Now, maybe I haven't understood the code, but have a look at: 
>> action.c, line 170
>> The function run_top_route() is called for the local_route. What 
>> happens if:
>>
>> route {
>> ...
>> setsflag(3);
>> if(method=="INVITE")
>>    m_dump();
>> if(issflagset(3))
>> {
>>   # is it going here?
>> }
>>
>> if(issflagset(5))
>> {
>>   # is it going here?
>> }
>>
>> ....
>> }
>>
>> local_route {
>>   setsflag(5);
>> }
>>
>> What is the expected behavior and is it acting properly now?
>>
>>>> - setbflag() broken
>>>> - resetbflag() broken
>>>> - isbflagset() broken
>>>>   
>>> This functions are not to be used from local route. But the core 
>>> functions do not have any flags to control the types of routes they 
>>> are called from (like the module exported functions).
>>> It is exactly the same case as for the onreply route.
>>
>> In case of onreply_route there is no side effect. In this case it is 
>> affected the environment of the SIP request processed in the route. 
>> If you look at the example above, if I set a branch flag in the local 
>> route, will it be visible in the 'route' after calling m_dump?
>>
>>
>>>> - initial scripts flags are lost, last local_route script flags are 
>>>> getting into initial route script flags
>>>>   
>>> the script flags are inherited from route to route by design - they 
>>> are not ever reset - same behaviour as for script variables.
>> The script flags are reset in the run_top_route() (see above), or I 
>> am wrong. So all script flags set in the route before m_dump() are 
>> lost and after, the script flags from the local_route are in place.
>>
>>>> - assignments of following pseudo variables is broken
>>>>     - $bf/$bF - branch flags
>>>>     - $sf/$sF - script flags
>>>>     - $br - adding branch
>>>>   
>>> As I said in the local route description email, the RURI, DSTURI and 
>>> BRANCHES are not to be changed (by design). Any such changes are 
>>> simple discarded or ignored.
>>> It is not broken - it simply does not work as not supposed to.
>>
>> Again, if I am not wrong (and I tested, but...), the branches for the 
>> MESSAGE request sent by m_dump are not changed, fine. The problem 
>> seems to be that the branches of the INVITE are changed.
>>
>>>> - usage of following pseudo-variables is broken
>>>>     - $Ts - time stamp
>>>>     - $Tf - formatted time
>>>>   
>>> they are not broken - as there is another msg->id, the function 
>>> behind these PVs will simply make a new lib call for time() in order 
>>> to return the current time - nothing is broken.
>>
>> I believe we have different opinions here. Let's suppose again the 
>> above example. INVITE is labeled with id 5, the MESSAGE by msilo is 
>> with id 6.
>>
>> If in the main route I want to see the time for the INVITE, I printed 
>> it gets cached for id 5. Then, due to m_dump() is going to 
>> local_route and printing the time for MESSAGE will create a new time 
>> caching for the id 6. Returning in the main route after m_dump() and 
>> printing again the time I may get something different although I am 
>> back in the context of the message id 5 -- as it was before the 
>> local_route, would have been the same. Now, by just plugging the 
>> local_route and printing the time there will break all the logic I 
>> have for 1.3 and previous versions.
>>
>>>>     - $br - branch
>>>>     - $bR - all branches
>>>>   
>>> I think these were already listed - branches are not allowed to be 
>>> used from the local route. You can add branches, you can see them 
>>> via PVs, but there will be not used.
>> What I meant here is that usage of these variables is broken. What if 
>> before m_dump() I add couple of branches for INVITE and in 
>> local_route I do:
>>
>> xlog("the branches for MESSAGE are: $bR\n");
>>
>> Do I get none or do I get the ones from the INVITE? Same for other 
>> cases, when I want to account $bR, what it gets for message and what 
>> for invite ...
>>
>>
>>>>     - $time(...) PV class from cfgutils is broken
>>>>   
>>> not broken - same comment as for $Ts and $Tf.
>> Same explanation as well.
>>
>>>> - avp_pushto("$br", ...) is broken
>>>> - avp_pushto("$ru", ...) with many avps for the second parameter is 
>>>> broken
>>>>   
>>> Again, changing RURI/DSTURI or adding branches is not allowed in the 
>>> route by design. See the above arguments for the corresponding PVs.
>>> Unfortunately the "avp_pushto" function  can operate with other PVs 
>>> (than $br and $ru), so it had to be enabled for local_route. But if 
>>> you use it for the above PVs, nothing will get broken - it will just 
>>> not do anything.
>> Aren't the branches of the INVITE actually affected?
>>
>>>> - setdebug() is broken
>>>>   
>>> it is not broken - it works exactly as in the other types of routes. 
>>> Have in mind that the debug level is set or reset only from script 
>>> and it is not ever automatically reset.
>> I need to investigate more here as could be just my current 
>> understanding about how the backup of the debug level is done and 
>> restored.
>>
>>>> - append_branch() is broken
>>>>   
>>> This function should not be used in local_route, but as it is a core 
>>> function, it cannot be explicitly blocked. Anyhow, even if you use 
>>> it, it will have no effect. It is exactly the same case as for 
>>> BRANCH ROUTE.
>> Again, it seems to affect the destination set for INVITE, not for 
>> MESSAGE.
>>
>

-- 
http://www.asipto.com