-
Notifications
You must be signed in to change notification settings - Fork 782
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
ifs log enhancements #4086
ifs log enhancements #4086
Conversation
krishanifs
commented
Sep 23, 2024
- In pj_assert put logs in non-debug modes
- Added PJ_ASSERT_LOG
- In pjsua_reconfigure_logging gets log file name from config
- Adding more logs
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.
Thank you for the contributions :)
#ifdef _WIN32 | ||
#define __FILENAME__ (strrchr(__FILE__, '\\') ? strrchr(__FILE__, '\\') + 1 : __FILE__) | ||
#else | ||
#define __FILENAME__ (strrchr(__FILE__, '/') ? strrchr(__FILE__, '/') + 1 : __FILE__) |
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.
- Better avoid macro names using underscore prefixes (read somewhere it maybe reserved for system, sdk, etc), e.g:
PJ_ASSERT_FILENAME
?
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.
Removed this in #4098 due to a couple of reasons:
- platform difficulty (as commented above).
- if we want to extract the filename only and get rid of its path, perhaps we need to do it in the log instead. It has been previously discussed in PJ_LOG_HAS_FILENAME #3889.
#ifndef pj_assert | ||
#include "pj/log.h" | ||
#include <string.h> | ||
#ifdef _WIN32 |
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.
For Windows, it is possible to use MinGW, so need to confirm whether MinGW compiler has _WIN32
and uses \\
instead of /
for path separator.
@@ -728,7 +728,7 @@ PJ_DEF(pj_status_t) pjsua_reconfigure_logging(const pjsua_logging_config *cfg) | |||
pjsua_logging_config_dup(pjsua_var.pool, &pjsua_var.log_cfg, cfg); | |||
|
|||
/* Redirect log function to ours */ | |||
pj_log_set_log_func( &log_writer ); | |||
pj_log_set_log_func( (cfg && cfg->cb && !cfg->log_filename.slen) ? cfg->cb : &log_writer ); |
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.
- We missed
cfg != NULL
verification, better add that, e.g:PJ_ASSERT_RETURN(cfg, PJ_EINVAL);
- Re: skipping the
log_writer
, it may change the behavior of the lib (potential backward compatibility issue), could you specify the reason?
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.
Done for the first one, the second one reverted to current code.
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.
We need to pass a different log_writer call back function that write pj logs into our own application logs. how are we going to achieve that without this implementation.
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.
The function log_writer() will call your callback.:
https://github.com/pjsip/pjproject/blob/master/pjsip/src/pjsua-lib/pjsua_core.c#L730
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 is an optimization we added. it directly call 'cfg->cb' if user has defined that; without calling it through log_writer. But it is OK to stick with current implementation.
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 continue this in #4098.
@@ -728,7 +728,7 @@ PJ_DEF(pj_status_t) pjsua_reconfigure_logging(const pjsua_logging_config *cfg) | |||
pjsua_logging_config_dup(pjsua_var.pool, &pjsua_var.log_cfg, cfg); | |||
|
|||
/* Redirect log function to ours */ | |||
pj_log_set_log_func( &log_writer ); | |||
pj_log_set_log_func( (cfg && cfg->cb && !cfg->log_filename.slen) ? cfg->cb : &log_writer ); |
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.
Done for the first one, the second one reverted to current code.
#ifdef _WIN32 | ||
#define __FILENAME__ (strrchr(__FILE__, '\\') ? strrchr(__FILE__, '\\') + 1 : __FILE__) | ||
#else | ||
#define __FILENAME__ (strrchr(__FILE__, '/') ? strrchr(__FILE__, '/') + 1 : __FILE__) |
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.
Removed this in #4098 due to a couple of reasons:
- platform difficulty (as commented above).
- if we want to extract the filename only and get rid of its path, perhaps we need to do it in the log instead. It has been previously discussed in PJ_LOG_HAS_FILENAME #3889.
Since we already create a separate PR to continue this, you need to create a new PR if you want to push a new commit, otherwise it will potentially cause conflicts when merged. |
sorry about the late commit but (41c0082) is also part of the log enhancement. |
* | ||
* @return PJ_SUCCESS on success, or the appropriate error code. | ||
*/ | ||
PJ_DECL(pj_status_t) pjsua_create2(const pjsua_logging_config *log_cfg); |
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.
Since we can pass the log config to pjsua_init(), why is this new API needed?