<p><b>@henningw</b> commented on this pull request.</p>
<p>Thank you also from my side for the contribution. I obviously only did a quick look to the code, given this complexity. I found a few places where it would be good to have a look.</p><hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/1912#discussion_r272636448">src/modules/siptrace/doc/siptrace_admin.xml</a>:</p>
<pre style='color:#555'>> @@ -18,24 +18,17 @@
<para>
The SIPtrace module offer a possibility to store incoming and outgoing SIP
messages in a database and/or duplicate to the capturing server (using <acronym>HEP</acronym>,
- the Homer encapsulation protocol, or plain SIP mode)
+ the Homer encapsulation protocol, or plain SIP mode). Since version 5.4 new levels of tracing
+ are available. Transactions and dialogs can be traced. Trace flag is now useless.
</pre>
<p>"flag is useless" - probably should be changed after the re-introducing as well.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/1912#discussion_r272636907">src/modules/siptrace/doc/siptrace_admin.xml</a>:</p>
<pre style='color:#555'>> @@ -656,6 +654,11 @@ hlog("$hdr(P-MyID)", "Another one with a custom correlation ID");
Stateless forwarded messages (forward()) are not logged if you set the
flag, use sip_trace() inside <emphasis>onsend_route</emphasis> block.
</para>
+ <para>
+ If dialog level tracing is used internally generated BYE transaction(in
+ case of internal timeouts) won't be traced. At the moment kamailio offers
+ no posibility to trace those messages.
</pre>
<p>Daniel commented in the main comment thread about the sip parsing, that it is actually not that expensive anymore</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/1912#discussion_r272637686">src/modules/siptrace/siptrace.c</a>:</p>
<pre style='color:#555'>> @@ -304,10 +338,30 @@ static int mod_init(void)
/* register callbacks to TM */
if(load_tm_api(&tmb) != 0) {
LM_WARN("can't load tm api. Will not install tm callbacks.\n");
- } else if(tmb.register_tmcb(0, 0, TMCB_REQUEST_IN, trace_onreq_in, 0, 0)
- <= 0) {
- LM_ERR("can't register trace_onreq_in\n");
- return -1;
+ }
+
+ if (load_dlg_api(&dlgb) < 0) {
+ LM_WARN("can't load dlg api. Will not install dialog callbacks.\n");
</pre>
<p>Below you log on ERROR level if the dialog tracing is not available, makes probably sense to log here with ERROR as well.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/1912#discussion_r272638096">src/modules/siptrace/siptrace.c</a>:</p>
<pre style='color:#555'>> /**
- * Send sip trace with destination and correlation id
+ * TODO TODO TODO:
</pre>
<p>Is this still a TODO or was already added?</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/1912#discussion_r272639151">src/modules/siptrace/siptrace.c</a>:</p>
<pre style='color:#555'>> @@ -1332,16 +1533,226 @@ static void trace_onreply_out(struct cell *t, int type, struct tmcb_params *ps)
sto.stat = siptrace_rpl;
#endif
- sip_trace_store(&sto, NULL, NULL);
- return;
+ if (info->uriState == STRACE_RAW_URI) {
+ LM_BUG("uriState must be either UNUSED or PARSED here! must be a bug! Message won't be traced!\n");
+ abort();
</pre>
<p>Calling abort() will kill Kamailio, it is not possible to recover here?</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/1912#discussion_r272639908">src/modules/siptrace/siptrace.c</a>:</p>
<pre style='color:#555'>>
- pkg_free(*param);
- /* free temporary proxy*/
- if(p) {
- free_proxy(p); /* frees only p content, not p itself */
- pkg_free(p);
+ trace_type = parse_siptrace_flag(&sflags);
+ if (trace_type == SIPTRACE_NONE) {
+ LM_ERR("Failed to parse trace type!\n");
+ return -1;
+ }
+
+ *param = pkg_malloc(sizeof(trace_type));
</pre>
<p>pkg_malloc can fail, check for success.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/1912#discussion_r272640752">src/modules/siptrace/siptrace.c</a>:</p>
<pre style='color:#555'>> }
- if(((char *)(*param))[0] == '\0') {
- // Empty URI, use the URI set at module level (dup_uri)
- if(dup_uri) {
- uri = *dup_uri;
- } else {
- LM_ERR("Missing duplicate URI\n");
- return -1;
- }
- } else {
- duri = (char *)*param;
+ for (idx = 0; idx < sflags->len; idx++) {
</pre>
<p>This looks that there a a lot of whitespaces, can you check? Maybe github shows it different.</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/kamailio/kamailio/pull/1912#pullrequestreview-223338198">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AF36ZTl_uo5OiMQSeqDcUSJ1H136aU7eks5vd22lgaJpZM4cP-3E">mute the thread</a>.<img src="https://github.com/notifications/beacon/AF36ZWc1DCDdXhWR72yjEI7sSk4ybO7pks5vd22lgaJpZM4cP-3E.gif" height="1" width="1" alt="" /></p>
<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/kamailio/kamailio","title":"kamailio/kamailio","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/kamailio/kamailio"}},"updates":{"snippets":[{"icon":"PERSON","message":"@henningw commented on #1912"}],"action":{"name":"View Pull Request","url":"https://github.com/kamailio/kamailio/pull/1912#pullrequestreview-223338198"}}}</script>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/kamailio/kamailio/pull/1912#pullrequestreview-223338198",
"url": "https://github.com/kamailio/kamailio/pull/1912#pullrequestreview-223338198",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>