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
> <mico...@gmail.com <mailto:mico...@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@lists.kamailio.org <mailto:sr-dev@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
_______________________________________________
Kamailio (SER) - Development Mailing List
sr-dev@lists.kamailio.org
https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev
