[sr-dev] siptrace inconsistency

[Daniel-Constantin Mierla]

Hello,

On 01.04.20 10:18, Federico Cabiddu wrote:
> Hi Daniel,
> thanks for the feedback. About your questions: received ACK and CANCEL
> were not captured in old versions of the module, so no changes from
> this point of view.
> But tracing and flagging every request would work without duplicated
> messages. Something like this
>
> request_route {
>     ....
>     if (!is_method("OPTIONS")) {
>         setflag(CAPTURE_FLAG);
>         sip_trace();
>     }
> }
>
> I would expect the legacy behavior preserved, but I need to check
> better the code to understand how to do. If not possible we should add
> it to the documentation.

OK, indeed, the previous behaviour should be preserved in this case. Is
sip_trace() without params now doing transaction mode capturing?


> I will make some tests removing the trace_is_off check for the
> negative ACK and make a PR.
OK, thanks for digging into it.
> Finally I think that we also should align case 3) (siptrace + flag)
> with case 1) (transaction tracing) so that the behavior is the same
> (as I think was the intention originally).
> What do you and other devs think?

I am fine with this.

Cheers,
Daniel


>
> Cheers,
>
> Federico
>
>  
>
> On Tue, Mar 31, 2020 at 4:25 PM Daniel-Constantin Mierla
> <miconda at gmail.com <mailto:miconda at gmail.com>> wrote:
>
>     Hi Federico,
>
>     were the received ack and cancel captured automatically in the old
>     version when sip trace was set for invite? There were many changes
>     in the past years, but I remember that the flag was mainly for
>     outgoing requests and matching replies of the transaction for
>     which the flag was set. For incoming requests sip_trace() function
>     had to be used.
>
>     Based on your remark, I think that trace_tm_neg_ack_in() should
>     not check if the trace-is-off(). It should be set when trace-is-on
>     and that's it for the transaction.
>
>     Feel free to clarify (or propose) the wanted behaviour and then we
>     can work together to have it as expected. I used sip trace lately
>     for tracing all traffic (trace_mode=1), no longer doing any
>     filtering for transactions/dialogs.
>
>     Cheers,
>     Daniel
>
>     On 31.03.20 09:09, Federico Cabiddu wrote:
>>     Hi all,
>>     I've been recently testing 5.3.x/master siptrace module, in
>>     particular the new trace mode "t" vs the legacy flag +
>>     sip_trace() mode and I've found some issues with the handling of
>>     CANCEL. Specifically, I've tested the following scenarios:
>>     1) sip_trace_mode("t") on the initial INVITE only: received ACK
>>     for negative replies not captured
>>     2) sip_trace_mode("t") on the initial INVITE and on neg ACK:
>>     received ACK captured twice
>>     3) setflag and sip_trace() on the initial INVITE only: received
>>     CANCEL and ACK not captured (outgoing yes)
>>     4) setflag and sip_trace() on the initial INVITE and ACK:
>>     received CANCEL not captured, received ACK captured twice
>>     5) setflag and sip_trace() for each message (legacy): received
>>     CANCEL and 200 captured twice, received ACK captured twice
>>
>>     Digging into the module's code the "culprit" looks to be
>>     trace_is_off function
>>     (https://github.com/kamailio/kamailio/blob/2768f8ce1cf6da242674e7e40c8e76eb6c630f6b/src/modules/siptrace/siptrace.c#L66)
>>     and the places where it is called.
>>     E.g.: for the case 1), when a negative reply is
>>     received, trace_tm_neg_ack_in is called, which calls inside
>>     trace_is_off
>>     (https://github.com/kamailio/kamailio/blob/2768f8ce1cf6da242674e7e40c8e76eb6c630f6b/src/modules/siptrace/siptrace.c#L1661),
>>     which cannot be true unless the ACK has been marked for capture
>>     in the script, in which case it will be capture twice (case 2).
>>     The same applies to the CANCEL for case 3), in trace_onreq_out
>>     (callback for TMCB_E2ECANCEL_IN) trace_is_off because the
>>     incoming message is not flagged. Case 3) should theoretically
>>     behave like case 1) according to
>>     commit https://github.com/kamailio/kamailio/commit/40e09d8625184f19ff5666a2848cbb8c6212db26.
>>
>>     I'm not really sure if (and how) modify the trace_is_off function
>>     or not calling it in specific cases. E.g.: why calling it
>>     in trace_tm_neg_ack_in? This callback is set when we explicity
>>     want to trace a transaction, so why checking inside if tracing is
>>     on? Maybe I'm missing something, but I think that probably the
>>     different behaviors of the modes should be better specified/decided.
>>
>>     Best regards,
>>
>>     Federico
>>
>>     _______________________________________________
>>     Kamailio (SER) - Development Mailing List
>>     sr-dev at lists.kamailio.org <mailto:sr-dev at lists.kamailio.org>
>>     https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev
>
>     -- 
>     Daniel-Constantin Mierla -- www.asipto.com <http://www.asipto.com>
>     www.twitter.com/miconda <http://www.twitter.com/miconda> -- www.linkedin.com/in/miconda <http://www.linkedin.com/in/miconda>
>
-- 
Daniel-Constantin Mierla -- www.asipto.com
www.twitter.com/miconda -- www.linkedin.com/in/miconda

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.kamailio.org/pipermail/sr-dev/attachments/20200401/3d7a8289/attachment-0001.html>