henningw commented on this pull request.
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.
@@ -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.
"flag is useless" - probably should be changed after the re-introducing as well.
@@ -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.
Daniel commented in the main comment thread about the sip parsing, that it is actually not that expensive anymore
@@ -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");
Below you log on ERROR level if the dialog tracing is not available, makes probably sense to log here with ERROR as well.
/**
- * Send sip trace with destination and correlation id + * TODO TODO TODO:
Is this still a TODO or was already added?
@@ -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();
Calling abort() will kill Kamailio, it is not possible to recover here?
- 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));
pkg_malloc can fail, check for success.
}
- 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++) {
This looks that there a a lot of whitespaces, can you check? Maybe github shows it different.