-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use different callback functions to get SIP log messages for Sofia-SIP 1.12 and Sofia-SIP 1.13. #3063
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I've added a few notes inline, mostly editorial. I'm wondering if the two functions can be merged in one that behaves differently depending on the version, but that's something that can be done later on if needed. Is this approach working fine for you, without the mixed lines you were experiencing at higher CPS?
return; | ||
char line[255]; | ||
char message[16000]; | ||
char sofia_log_buf[16000]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These buffers are way too large.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I set those values to match the maximum buffer that Sofia SIP defines in https://github.com/freeswitch/sofia-sip/blob/master/libsofia-sip-ua/tport/tport_logging.c#L843
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's still too large of a buffer, especially to create any time the function is called. The SIP SDPs in Janus are never very large, so I'd put a cap on it. PR #3060 for instance set it to 3072 because in their experience the buffer never went beyond that.
index++; | ||
continue; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty line is unneeded here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove it
} | ||
|
||
/* First line of the log indicates if we send or receive a message. */ | ||
/* We are only interested in the sent ones */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"send or receive" should be "sent or received I guess?
Just a nit per code style: use a single comment block, not two, e.g.
/* First line of the log indicates if we sent or received a message:
* we are only interested in the sent ones */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix both.
g_strfreev(parts); | ||
return ; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unneeded empty line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove it
return ; | ||
} | ||
|
||
if(strlen(line) == 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lminiero pedantic warning that's not exactly equivalent (note that in #3059 we only changed calls doing greater than or less than comparisons since the results were guaranteed not to change) although AFACT it should be safe to change this like you say:
strnlen("f", 1 + 1): 1
strlen("f"): 1
strnlen("foo", 1 + 1): 2
strlen("foo"): 3
strnlen("", 1 + 1): 0
strlen(""): 0
continue; | ||
} else { | ||
/* Is this a Call-ID header? Keep note of it */ | ||
if(strstr(line, "Call-ID") == line) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per code style, since the action is simple there's no need for curly brackets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part already exists in the janus_sip_sofia_logger callback function (the one for line-by-line logs). I can fix it in both functions if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ouch, I ended up reviewing my own ugly code 🤭
Please update both, thanks!
} else { | ||
/* Is this a Call-ID header? Keep note of it */ | ||
if(strstr(line, "Call-ID") == line) { | ||
g_snprintf(call_id_hdr, sizeof(call_id_hdr), "%s", line+9); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this 9? Are you assuming Call-ID:
? The strstr
call above is looking for Call-ID
, not Call-ID:
, which can cause problems if the header is formatted differently, or if Call-ID
is not followed by a semicolon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also existed in janus_sip_sofia_logger. I suppose it's safe to assume that "Call-ID: " will always be sent with this format, as it is Sofia-SIP library the one that generates those messages and we only process the messages sent from Sofia SIP stack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense!
/* Look for the session this message belongs to */ | ||
janus_sip_session *session = NULL; | ||
janus_mutex_lock(&sessions_mutex); | ||
if(strlen(call_id_hdr)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be changed to
if(strlen(call_id_hdr) > 0)
otherwise I'm not sure if the compiler optimizations discussed in #3059 will kick in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a cursory check of a few compilers (e.g., https://godbolt.org/z/enodKdhMc) if(strlen(call_id_hdr))
and if(strlen(call_id_hdr) > 0)
will both compile down to the same check of call_id_hdr[0] != 0
So you can use whichever you find more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking!
Adding the multistream label as the patch is for master. I'll take care of backporting to |
@oriol-c any update on the fixes? As soon as they happen, I'll make some tests and in case merge. Thanks! |
@oriol-c please let us know if you've abandoned plans to update this PR. In case, we'll just close it. Thanks. |
Use different callback functions to get SIP log messages for Sofia-SIP 1.12 and Sofia-SIP 1.13.
In Sofia-SIP 1.12 the callback was called for each line in the SIP message. In Sofia-SIP 1.13 this behavior was changed to send the entire SIP message in a single callback.
This could also be implemented using macros in the Makefile and setting the Sofia SIP version in the compile runtime, but that would remove the flexibility of the compiled Janus to run simultaneously against both Sofia SIP versions.