@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.

In src/modules/siptrace/doc/siptrace_admin.xml:

> @@ -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.

In src/modules/siptrace/doc/siptrace_admin.xml:

> @@ -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

In src/modules/siptrace/siptrace.c:

> @@ -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.

In src/modules/siptrace/siptrace.c:

>  /**
- * Send sip trace with destination and correlation id
+ * TODO TODO TODO:

Is this still a TODO or was already added?

In src/modules/siptrace/siptrace.c:

> @@ -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?

In src/modules/siptrace/siptrace.c:

>  
-	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.

In src/modules/siptrace/siptrace.c:

>  	}
 
-	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.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.