[sr-dev] Is there a sense in calling run_onsend() after the msg_send() function?
Lucian Balaceanu
lucian.balaceanu at 1and1.ro
Tue Dec 9 13:15:03 CET 2014
Hi Daniel,
Taking in considerations your input I did some changes to the previous
proposal:
1. I used the SEND_PR_BUFFER wrapper for sending out all messages
2. I moved the declaration of "struct ip_addr ip" variable towards the
start of the relay_reply() function and removed the other style
infringing variable send_res ;
3. I safeguarded the run_onsend() call with checking first that p_msg is
neither NULL nor set to FAKED_REPLY.
As far as I understand, FAKED_REPLY should be used when automatically
locally generating a reply, right? Additionally, locally generated
replies which are triggered from the config via the t_reply() function
are not processed here. However, only the code which passes through the
t_reply() is served by the event_route[tm:local-request], right?
Would you say that the current patch addresses all the statefully
relayed replies cases?
Apart from this, without some more knowledgeable input, I don't know any
other unifying solution for this patch (e.g. call onsend route for both
forwarded and local generated messages as you suggested).
Thank you,
Lucian
On 12/04/2014 12:58 AM, Daniel-Constantin Mierla wrote:
> Hello,
>
>
> On 03/12/14 18:14, Lucian Balaceanu wrote:
>> Hi Daniel,
>>
>> Indeed, locally generated replies provoke a segmentation fault in my
>> code because of the missing msg pointer in such cases.
>> I am currently testing a version where I generate the msg from the
>> buf to be sent by using the parse_msg() function.
>> Conceptually, do you see any problems with this approach?
>
> there are other attributes that might not be set in this case, such as
> source ip/port, local ip/port...
>
> Cheers,
> Daniel
>
>>
>> Thank you,
>> Lucian
>>
>> On 11/27/2014 10:37 AM, Daniel-Constantin Mierla wrote:
>>> Hi Lucian,
>>>
>>> the patch is still in radar, just that last month was crazy busy
>>> from various aspects and didn't get the time to handle it.
>>>
>>> On 27/11/14 09:09, Lucian Balaceanu wrote:
>>>> Hi Daniel,
>>>>
>>>> I must admit this run_onsend() patch for stateful replies creation
>>>> was not quite a success story. However, I think it serves a
>>>> practical purpose, for example in Homer tracing and could be useful
>>>> for the community. Again, I propose my past solution, with some
>>>> questions:
>>>>
>>>> 1. I am unsure if the place I introduced the run_onsend call is
>>>> appropriate since the buf used for msg_send is constructed
>>>> build_res_buf_from_sip_req and build_res_buf_from_sip_res calls.
>>>
>>> These functions are to build the reply from request (local generated
>>> reply) or from the incoming reply (forwarding reply).
>>>
>>> I am wondering what would take to call onsend route for both
>>> forwarded and local generated messages (no matter is request or
>>> reply) -- might become simpler by having the code in the wrapper
>>> function sending to the network from core, on the other hand could
>>> break scripting as no msg structure is available for local generated
>>> messages. Otherwise the benefit can be also in coherency.
>>>
>>>
>>>>
>>>> 2. Also, we can maybe unite these if call branches I created:
>>>> send_res = msg_send(&uas_rb->dst, buf, res_len);
>>>> send_res = SEND_PR_BUFFER( uas_rb, buf, res_len );
>>>
>>> SEND_PR_BUFFER seems to be a wrapper around msg_send() for safety
>>> and debugging processes, so it can be used.
>>>
>>>>
>>>> 3. Do you think a get_send_socket snippet as follows should be
>>>> inserted before the /*if (onsend_route_enabled(SIP_REPLY)){*/ :
>>>>
>>>> if(dst.send_sock == NULL) {
>>>> dst.send_sock=get_send_socket(msg, &dst.to, dst.proto);
>>>> if (dst.send_sock==0){
>>>> LM_ERR("cannot forward reply\n");
>>>> }
>>>> }
>>>
>>> Is the above referring to the patch attached? If the send_sock is
>>> needed elsewhere after the IF block, then should be inserted before.
>>>
>>> One remark, new variables should be declared at the beginning of the
>>> blocks, you mixed the style with two new vars you added -- this is
>>> more a suggestions as we try to stay compatible with old c standards
>>> -- I know it is not everywhere in the code, but we should aim at it.
>>>
>>> Cheers,
>>> Daniel
>>>>
>>>> Thank you,
>>>> Lucian
>>>>
>>>> On 10/29/2014 02:15 PM, Daniel-Constantin Mierla wrote:
>>>>> Hello Lucian,
>>>>>
>>>>> I applied your patch with some fixes.
>>>>>
>>>>> I haven't checked with stateful replies, at some point a function
>>>>> from core should be used. You can go ahead and see if it works, if
>>>>> not, let me know and I can look into it as well. You can follow
>>>>> the callbacks for TMCB_RESPONSE_OUT or TMCB_RESPONSE_FWDED inside
>>>>> tm code, they should lead to the place where a sip response is
>>>>> going to be sent out.
>>>>>
>>>>> Cheers,
>>>>> Daniel
>>>>>
>>>>> On 27/10/14 12:51, Lucian Balaceanu wrote:
>>>>>> Hello Daniel,
>>>>>>
>>>>>> I must admit I only saw your mail last Friday. Until the 10th of
>>>>>> October I was also on vacation. I know that you actually
>>>>>> committed some of the changes together with your comments on the
>>>>>> 12th this month.
>>>>>>
>>>>>> I don't know if we can consider the topic of the patch closed. As
>>>>>> far as I understand, the state-full replies have not been
>>>>>> addressed, right? (There should be a change in the t_reply.c) I
>>>>>> followed the code to the relay_reply but I did not yet come to
>>>>>> find the send function. Should I pursue further?
>>>>>>
>>>>>> Thank you,
>>>>>> Lucian Balaceanu
>>>>>>
>>>>>>> Hi Lucian,
>>>>>>>
>>>>>>> somehow I forgot to follow up on this. But we need to get sorted
>>>>>>> out soon, before we release, so it works as expected with the
>>>>>>> new version. See more comments inline.
>>>>>>>
>>>>>>>
>>>>>>> On 17/09/14 18:09, Lucian Balaceanu wrote:
>>>>>>>> Hi Daniel,
>>>>>>>>
>>>>>>>> Please forgive me for my delay in responding to your mail.
>>>>>>>> Please find attached a second version of the onsend_route_reply
>>>>>>>> patch (which again has some problems). As per your previous
>>>>>>>> indications I did the following:
>>>>>>>>
>>>>>>>> *Issue1*
>>>>>>>>> From performances point of view, there can be added a config
>>>>>>>>> parameter to enable running of onsend_route for replies:
>>>>>>>>>
>>>>>>>>> onsend_route_reply = 0|1
>>>>>>>>
>>>>>>>> Following
>>>>>>>> http://www.asipto.com/pub/kamailio-devel-guide/#c08add_parameters
>>>>>>>> I have tried to add onsend_route_reply parameter. The code
>>>>>>>> compiles, but when trying to start kamailio with this parameter
>>>>>>>> inside, the parsing fails with syntax errors signaling:
>>>>>>>>
>>>>>>>> / 0(1321) :<core> [cfg.y:3423]: yyerror_at(): parse error in
>>>>>>>> config file kamailio-basic.cfg.4.1, from line 107, column 1 to
>>>>>>>> line 108, column 0: syntax error
>>>>>>>> 0(1321) : <core> [cfg.y:3423]: yyerror_at(): parse error in
>>>>>>>> config file kamailio-basic.cfg.4.1, from line 107, column 1 to
>>>>>>>> line 108, column 0:
>>>>>>>> ERROR: bad config file (2 errors)/
>>>>>>>
>>>>>>> The issue is:
>>>>>>>
>>>>>>> +<INITIAL>{ONSEND_RT_REPLY} { yylval.intval=atoi(yytext);
>>>>>>> + yy_number_str=yytext; return NUMBER; }
>>>>>>>
>>>>>>> It should be:
>>>>>>>
>>>>>>> +<INITIAL>{ONSEND_RT_REPLY} { yylval.intval=atoi(yytext);
>>>>>>> + yy_number_str=yytext; return ONSEND_RT_REPLY; }
>>>>>>>
>>>>>>>>
>>>>>>>> *Issue2*
>>>>>>>>> #define onsend_enabled(rtype)
>>>>>>>>> (onsend_rt.rlist[DEFAULT_RT]?((rtype==SIP_REPLY)?onsend_route_reply:1):0)
>>>>>>>> That is to say you see it best to take the chek for
>>>>>>>> onsend_rt.list[DEFAULT_RT] from inside run_onsend() function
>>>>>>>> and call this onsend_enabled(...) before the run_onsend()?
>>>>>>>
>>>>>>> This is to detect whether the onsend_route should be executed
>>>>>>> for SIP replies. The condition being:
>>>>>>>
>>>>>>> - if is a sip reply and onsend_route is set and the
>>>>>>> onsend_route_reply parameter is 1
>>>>>>>>
>>>>>>>> *Issue3*
>>>>>>>>> On the other hand, is onsend_route also executed for local
>>>>>>>>> requests? I had in mind it is only for received requests that
>>>>>>>>> are forwarded ... Iirc, on onsend_route, the sip message is
>>>>>>>>> the one received, the outgoing content being accessible via
>>>>>>>>> $snd(buf).
>>>>>>>>>
>>>>>>>> I agree with you with taking out the locally generated requests
>>>>>>>> and only left the run_onsend call in do_forward_reply function
>>>>>>>> (inside forward.c).
>>>>>>>> Could you point me to the reply relaying function that is
>>>>>>>> called for state-full processing?
>>>>>>> Stateful processing for replies is mainly done in t_reply.c from
>>>>>>> tm module. At some point there should be a send buffer function
>>>>>>> call.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Daniel
>>>>>>>>
>>>>>>>> Thank you and sorry again for my late answer,
>>>>>>>> Lucian
>>>>>>>
>>>>>>> --
>>>>>>> Daniel-Constantin Mierla
>>>>>>> http://twitter.com/#!/miconda -http://www.linkedin.com/in/miconda
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>> --
>>>>> Daniel-Constantin Mierla
>>>>> http://twitter.com/#!/miconda -http://www.linkedin.com/in/miconda
>>>>
>>>
>>> --
>>> Daniel-Constantin Mierla
>>> http://twitter.com/#!/miconda -http://www.linkedin.com/in/miconda
>>
>
> --
> Daniel-Constantin Mierla
> http://twitter.com/#!/miconda -http://www.linkedin.com/in/miconda
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sip-router.org/pipermail/sr-dev/attachments/20141209/14af1c79/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: onsend_route_reply_tm.patch
Type: text/x-patch
Size: 2815 bytes
Desc: not available
URL: <http://lists.sip-router.org/pipermail/sr-dev/attachments/20141209/14af1c79/attachment-0001.bin>
More information about the sr-dev
mailing list