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

Thu Nov 27 09:37:53 CET 2014

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

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