[OpenSER-Devel] SF.net SVN: openser: [4329] trunk/modules/benchmark/benchmark.c

[Bogdan-Andrei Iancu]

Daniel,

Not sure what make you say it is a quick hack... I admit it is not the 
final version of the idea, but this is the first step, trying to solve 
the most urged problems - if you want to call a quick hack anything that 
solves something, fine with me.

It is the first step as in the next version, transaction persistence 
(like being able to use onreply / failure / branch  routes) it will be 
added - right now I had no time to do the changes into TM in order to 
store the fake sip_msg for later usage. This requires some more complex 
changes that requires more time.

Also, see me inline comments on the your technical remarks.

Regards,
Bogdan


Daniel-Constantin Mierla wrote:
>
>
> On 06/06/08 21:04, Bogdan-Andrei Iancu wrote:
>> Hi Henning,
>>
>> What do you mean by "we have to live with it" ? it sounds like 
>> something terrible wrong happened.
>>
>> To be honest  I fail to see the logic behind of this. You and Daniel 
>> are complaining about some new code - based on a superficial 
>> understanding of the code and functionality you start formulating 
>> some gratuitous  opinion, but without bringing any real technical 
>> arguments for this opinion.
>>
>> I consider truly fair and respectful for a collective effort to :
>>    - try understand what is the idea and the code before expressing 
>> any disapproval.
>>    - if you are not able to understand, please ask the developer to 
>> clarify the issues for you - better that making wrong assumptions.
> If you do read my previous email you will understand what I meant. 
> This new stuff looks like quick hack to fix just few demands, whilst a 
> discussion about such major change (done in the last minute) would 
> reveal a better design and approach. openser is now beyond one man 
> interests, and as you talk about collective work and collaboration, 
> what happened it is not the right approach.
[bogdan]
If you have/had a better solution, nobody stopped you to implement it ;) 
. The idea about this route was advertised and discussed for some time 
over the tracker and mailing list. Unfortunately there was no input from 
your side.

>
> I do understand all the code, and really don't like the solution. What 
> it brings:
> - for internally generated messages we can now: do xlog, add headers, 
> benchmarking, avpops and accounting
[bogdan]
these were the first functionality enabled for this route. As said, the 
work regarding this route is not yet completed. For the moment is brings 
solution for the most demanded features that were discussed on tracker 
and mailing list.
>
> The new route is executed after the buffer with the message (that was 
> previously sent to the network) is built,
[bogdan]
actually the route is executed before sending anything on the net - any 
processing after that makes no sense.
> reparsed in a local static sip_msg structure, and then apply 
> eventually new changes induced from local_route, building a new buffer 
> and sending. No real connection to TM, just that the function to send 
> such message is there
[bogdan]
that is the whole idea and purpose - the route is called when TM sends 
an internal request. I think it is quite a strong connection :)
> -- no possibility to bind to transaction callbacks, or other stuff 
> specific to TM.
[bogdan]
it is not yet there, but (as said) it will be in the next version. 
Simple, it was not time for this complex task.
>
> Same, but more will be available if it is going to be plugged in the 
> core, for all messages.
[bogdan]
as the route is intended only for internally generated requests, TM is 
the only one doing it, so is will be useless for core - the core does 
not provide no functionality to send requests/replies without script 
interaction. And if you have script interaction, there is no need for 
another special route.
> Self generated messages are easy to detect, by that we can select the 
> ones that are processed with the current version. More over:
> - siptrace can be easily updated to avoid tm callbacks and use this 
> new route for outgoing messages, and it will work even for stateless mode
> - the destination ip/port/protocol can be made availabe in just few 
> lines of code, allowing flexible filtering and accounting (there is a 
> pending feature request IIRC)
> - details from sent buffer may be accounted
[bogdan]
all this proves me that unfortunately you missed the point with this 
route. This route does not intend to provide access to message buffer or 
other info filled before putting the message on the net (as you 
suggest). It simple provides script access to requests internally 
generated by TM (which otherwise are transparent for the script).
Maybe the fact that the route is building a sip_msg (to provide it to 
the script functions) from the script tricked you - but when TM builds a 
new brand requests, it directly builds the buffer without no sip_msg.

siptrace does not generate any new request - it is just dumping the 
message buffer.
destination ip/port/protocol - not related to any of this.
sent buffer - again, it is not the purpose of this route.

As, said, my feeling is that you made some confusion on the route 
purpose - it is not intended to provide access to the buffer or IP info 
just before pushing the message on the net - it simple provide script 
access for some requests that didn't had any; it is not about any 
buffer, as the route provides a sip_msg to the functions.
> - the self generated CANCELs can be handled as well.
[bogdan]
yes - already discussed with Carsten Bock and this will be covered by 
the next version (see my email to him) - internal generated CANCELs are 
bit too delicate to catch and process in a route (as they are part of 
the INVITE transaction)
> Just some that got in my mind.
>
>>
>>
>> Again, I open to any technical comments on the code, if any.
>
> You mean about the C code or? Nobody claimed current code is wrongly 
> written, just it is not the proper solution for project's demands.
[bogdan]
how do you define the demand of the project ? because this new route was 
a direct result of needs of the several people. Usually I tent to 
consider the users needs as the project needs.
If wrong, any input is welcome.
>
> Cheers,
> Daniel
>>
>> Regards,
>> Bogdan
>>
>>
>> Henning Westerholt wrote:
>>> On Friday 06 June 2008, Daniel-Constantin Mierla wrote:
>>>  
>>>> [..]
>>>> I have to second Henning here in regard to a proper discussion may 
>>>> lead
>>>> to better solution. Maybe I am missing something, but from what I have
>>>> seen in the description and the code, binding this route to TM solve
>>>> just few of the demands. It is called after the buffer of the 
>>>> message to
>>>> be sent is built.
>>>>
>>>> IMO such route shall be available for all messages sent by openser, so
>>>> we get access in it to what is going to be sent on the network. There
>>>> are solutions to detect that the message is locally generated (e.g.,
>>>> only one via header), to decide there what to do. Apart of the 
>>>> benefits
>>>> brought now, siptrace will work for all messages, we can get make
>>>> available the destination ip and port, therefore filtering is more
>>>> flexible, also logging/accounting get access to this information.
>>>>
>>>> I believe that this is a real topic for discussion, and if it is 
>>>> really
>>>> important, we can postpone one or two days the freeze for the right
>>>> decision. Leaving a half solution for a full release is not a right
>>>> approach.
>>>>     
>>>
>>> Hello Daniel,
>>>
>>> i agree here to you, of course. But i think that further delaying of 
>>> the freeze is also not a good solution. So i think we'll probably 
>>> need to live with this solution for 1.4.
>>>
>>> Cheers,
>>>
>>> Henning
>>>
>>>
>>>   
>>
>