[OpenSER-Devel] discussion: issues with local_route
Daniel-Constantin Mierla
miconda at gmail.com
Wed Jun 11 18:47:55 CEST 2008
Hello,
On 06/11/08 18:06, Bogdan-Andrei Iancu wrote:
> Hi Daniel,
>
> Thanks for the feedback - see my inline comments for the cases you
> identified.
I see missing lot of my comments, shall be started in a different thread
or they were ignored just for this conversation? I was saying that the
commit did affect the design and the architecture. And it does that and
breaks many things. I won't carry on with a new major release having
attached a big log of "don't do that" even it is allowed, the results
are totally different than should be normally.
I hope is more clear why I got no more in the discussions from my last
email on Friday evening, since Friday I just said it looks like major
change and it is fair to let the others the chance to review and see
what was affected and how, bring ideas and improvements. I didn't mean
to enable the LOCAL_ROUTE flags for the modules, but let the developers
give a thought about. I believe that is fair enough for everybody.
Now going further, I will provide more details...
>
> Regards,
> Bogdan
>
> Daniel-Constantin Mierla wrote:
>> However, here is a list with what I find subject for discussion, to
>> be broken, not properly behavior or misleading (considering the
>> timeline of last Friday evening):
>> - exit or drop have no effect, misleading because of their meaning
>>
> exit() works as in the other routes - it terminates the route execution
>
> drop() is interpreted depending of the type of route:
> REQUEST - does nothing
> ONREPLY - depending of the reply code
> BRANCH - drops the current brach
> FAILURE - does nothing
> for LOCAL, it does nothing by design , but I plan to add in the future
> the possibility to drop the request.
So it is why actually I brought into discussion how these are
interpreted in the local_route. They are allowed there and from my point
of view, local_route is just processing another SIP request, the one
generated internally. The ;route' is for processing SIP requests
received from the network. Being pretty much similarity, the behavior
for exit/drop shall be coherent and agreed by developers and implemented
before a major release.
>
>> - 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.
>> - pv_printf(br, ...) (avp_printf(br, ...)) is broken
>>
> branches are not supposed to work in the local route, as in some other
> routes (like branch route). If you found that using them may brake
> something, please let me know.
See previous comment.
>> - have no time to check all the source code, but other developers can
>> see if they use that -- the sip_msg->id is changed, so if you call a
>> function that checks it to see if some cached data is still valid
>> because it is same message you processed by the call of same function
>> or another one, once in the initial route, then in the nested
>> local_route, when you need it again in the initial route, the cached
>> value is considered invalid although it is the message in current
>> processing
>>
>>
> That is the whole purpose with the msg->id - if the message is other,
> refresh the cache. If you identified some cases (other than the one
> listed) where things get broken, please let me know.
The one with the time PVs are the first identified case, and local_route
broke the current behavior.
I had other subjects brought into discussions, which I find very
important to be discussed before going further with the decision. I
welcome the other developers to add their opinion or ask if something is
not clear in what I wrote.
I can summarize that the major problem is that many functions and
operations were not designed to work in nested SIP request contexts.
Although the impression is that they work on the MESSAGE context,
affecting the variables around this request, they affect the ones
related to INVITE, and the results may be unexpected.
I can provide some small scripts to see that operations done in
local_route are affecting the INVITE although should not.
Cheers,
Daniel
>
>
>
--
http://www.asipto.com
More information about the Devel
mailing list