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

[Daniel-Constantin Mierla]

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.

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

The new route is executed after the buffer with the message (that was 
previously sent to the network) is built, 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 -- no 
possibility to bind to transaction callbacks, or other stuff specific to TM.

Same, but more will be available if it is going to be plugged in the 
core, for all messages. 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
- the self generated CANCELs can be handled as well.
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.

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
>>
>>
>>   
>

-- 
http://www.asipto.com