[sr-dev] Is there a sense in calling run_onsend() after the msg_send() function?

Daniel-Constantin Mierla miconda at gmail.com
Mon Sep 7 16:39:40 CEST 2015 

Hi Lucian,

finally managed to push this patch -- I forgot it for a while, being
brought back by a discussion on sr-users.

You patch was committed as you made it:

  -
https://github.com/kamailio/kamailio/commit/2690a8c314d23406649dceaadce7032690500a6e

Then I pushed another to actually not send out if drop was used inside
the onsend_route:

  -
https://github.com/kamailio/kamailio/commit/4b2d6dd7ce1a61c964f7d996c2db4428010dd478

Hopefully it matches what you expected to get, any testing and feedback
is appreciated.

Cheers,
Daniel

On 09/12/14 13:15, Lucian Balaceanu wrote:
> 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
>

-- 
Daniel-Constantin Mierla
http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda
Book: SIP Routing With Kamailio - http://www.asipto.com
Kamailio Advanced Training, Sep 28-30, 2015, in Berlin - http://asipto.com/u/kat

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sip-router.org/pipermail/sr-dev/attachments/20150907/ed6e7146/attachment-0001.html>

More information about the sr-dev mailing list