[sr-dev] Is there a sense in calling run_onsend() after the msg_send() function?
Daniel-Constantin Mierla
miconda at gmail.com
Wed Dec 3 23:58:06 CET 2014
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/20141203/b6f4e9eb/attachment-0001.html>
More information about the sr-dev
mailing list