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

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?

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