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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/1912#pullrequestreview-223338198_______________________________________________
Kamailio (SER) - Development Mailing List
[email protected]
https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev
