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
_______________________________________________ sr-dev mailing list [email protected] http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
