Skip to content

Signed-off-by: Pesala Silva <[email protected]> Log_Enhacements for pjproject 2.13#4546

Closed
PesalaDeSilva wants to merge 3 commits intopjsip:masterfrom
PesalaDeSilva:log_enhancement_PJSIP_2.13
Closed

Signed-off-by: Pesala Silva <[email protected]> Log_Enhacements for pjproject 2.13#4546
PesalaDeSilva wants to merge 3 commits intopjsip:masterfrom
PesalaDeSilva:log_enhancement_PJSIP_2.13

Conversation

@PesalaDeSilva
Copy link
Copy Markdown
Contributor

This PR for the Log Enhancements and modifications done for logs for better information for PJSIP 2.13 version

@trengginas trengginas requested a review from Copilot August 19, 2025 08:24
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances logging functionality in PJSIP 2.13 by improving log message detail and fixing log function configuration. The changes add call ID and destination URI information to transport messages and allow proper callback function usage in logging configuration.

  • Enhanced SIP message logging with additional Call-ID and To header information
  • Fixed log function configuration to properly handle custom callback functions
  • Minor code formatting cleanup (whitespace removal)

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
pjsip/src/pjsua-lib/pjsua_core.c Fixes log function configuration to use custom callback when appropriate
pjsip/src/pjsip/sip_transport.c Enhances message logging with Call-ID and To header information
pjlib/src/pj/os_core_win32.c Adds trailing newlines for formatting consistency
pjlib/include/pj/assert.h Removes unnecessary blank line

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

const pjsip_msg *msg)
{
char info_buf[128], *info;
char info_buf[256], to_uri[PJSIP_MAX_URL_SIZE], * info;
Copy link

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The spacing around the asterisk in '* info' is inconsistent with C style conventions. It should be 'char info_buf[256], to_uri[PJSIP_MAX_URL_SIZE], *info;' with the asterisk attached to the variable name.

Suggested change
char info_buf[256], to_uri[PJSIP_MAX_URL_SIZE], * info;
char info_buf[256], to_uri[PJSIP_MAX_URL_SIZE], *info;

Copilot uses AI. Check for mistakes.
cid = (const pjsip_cid_hdr*)pjsip_msg_find_hdr(msg, PJSIP_H_CALL_ID, NULL);
to = (const pjsip_to_hdr*)pjsip_msg_find_hdr(msg, PJSIP_H_TO, NULL);
len = (to && to->uri) ? pjsip_uri_print(PJSIP_URI_IN_FROMTO_HDR, to->uri, to_uri, sizeof(to_uri)) : 0;
to_uri[((len > 0) && (len < sizeof(to_uri))) ? len : 0] = '\0';
Copy link

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The complex ternary expression makes the null termination logic difficult to read. Consider breaking this into clearer conditional logic: 'if (len > 0 && len < sizeof(to_uri)) { to_uri[len] = '\0'; } else { to_uri[0] = '\0'; }'

Suggested change
to_uri[((len > 0) && (len < sizeof(to_uri))) ? len : 0] = '\0';
if (len > 0 && len < (int)sizeof(to_uri)) {
to_uri[len] = '\0';
} else {
to_uri[0] = '\0';
}

Copilot uses AI. Check for mistakes.
cseq->cseq,
cid ? "/cid=" : "", cid ? (int)cid->id.slen : 0, cid ? cid->id.ptr : "",
to_uri[0] ? "/to=" : "", to_uri,
obj_name);
Copy link

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The format string with multiple consecutive %s specifiers makes it difficult to understand the output format. Consider using more descriptive format strings or breaking this into multiple sprintf calls for better readability.

Copilot uses AI. Check for mistakes.
@trengginas
Copy link
Copy Markdown
Member

I think the added information is a bit excessive. The detail message content can be printed by enabling pjsua_logging_config::msg_logging.

@sauwming
Copy link
Copy Markdown
Member

Thank you for the PR submission.

We’ve accepted most of the log enhancement contributions in #4086. However, regarding the additional SIP transport log in this PR, we believe it’s best to keep the current behavior. While the change does provide more detailed information, it also introduces extra overhead in terms of printing and memory buffer required.

If your application requires more detailed logging, we recommend enabling msg_logging as @trengginas suggested above. For further customization, you can also install your own log writer, either through the logging callback or by using the PJSIP module (this way, you don't need to modify PJSIP source base). For example, in pjsua the message logging will look like this:

13:36:16.747           pjsua_core.c  ....TX 1002 bytes Request msg REGISTER/cseq=41884 (tdta0x621000044db0) to TLS 139.162.62.29:5061:
REGISTER sip:sip.pjsip.org;transport=tls SIP/2.0
Via: SIP/2.0/TLS 199.77.210.25:64167;rport;branch=z9hG4bKPjC5A1C056-1E55-4F7C-B8E1-66CA46A5562F;alias
Route: <sip:sip.pjsip.org;transport=tls;lr>
Max-Forwards: 70
From: "Ming/401" <sips:[email protected]>;tag=736D4225-AB48-442F-8522-16B4F2FBED73
To: "Ming/401" <sips:[email protected]>
Call-ID: FAFB9CDE-C64F-40D0-BB71-3ED4831FD255
CSeq: 41884 REGISTER
....

@sauwming
Copy link
Copy Markdown
Member

Another alternative is to introduce a compile-time setting such as PJSIP_MSG_INFO_HAS_EXTRA_DETAILS (default is off for efficiency purposes).
So let us know whether:
a. you prefer to move the logging into your application level and leave the PJSIP code unchanged
b. modify the PJSIP code as per your proposed patch but add the compile-time setting above.

If you choose option b, there's nothing to do on your part. We will create a new PR based on your PR. If you choose a, then we will close the PR.

@sauwming
Copy link
Copy Markdown
Member

For the patch with the additional compile-time option, please check #4559 and #4560.

@sauwming
Copy link
Copy Markdown
Member

Completed with additional compile-time option in #4560

@sauwming sauwming closed this Aug 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants