[sr-dev] Is there a sense in calling run_onsend() after the msg_send() function?
Lucian Balaceanu
lucian.balaceanu at 1and1.ro
Tue Sep 8 08:51:16 CEST 2015
Hi Daniel,
Thank you for this patch and the changes. I will bring here the issues,
if any, related to it.
Have a nice day,
Lucian
On 07.09.2015 17:39, Daniel-Constantin Mierla wrote:
> 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/20150908/6f30fb88/attachment-0001.html>
More information about the sr-dev
mailing list