[Asterisk-code-review] res pjsip/config system: Add ast sip get pjproject buildopt (asterisk[13])
Richard Mudgett
asteriskteam at digium.com
Fri Jan 15 15:44:11 CST 2016
Richard Mudgett has posted comments on this change.
Change subject: res_pjsip/config_system: Add ast_sip_get_pjproject_buildopt
......................................................................
Patch Set 5: Code-Review-1
(13 comments)
https://gerrit.asterisk.org/#/c/2012/5/include/asterisk/res_pjsip.h
File include/asterisk/res_pjsip.h:
Line 2199: int ast_sip_get_pjproject_buildopt(char *option, char *format_string, ...) __attribute__((format(printf, 2,0)));
__attribute__((format(printf, 2, 3)))
https://gerrit.asterisk.org/#/c/2012/5/res/res_pjsip/config_system.c
File res/res_pjsip/config_system.c:
Line 152: static pj_log_func *log_cb_orig;
: static unsigned decor_orig;
These shouldn't be necessary since res_pjsip_log_forwarder.c saved these to restore the values on unload. res_pjsip.so effectively owns the pjlib instance.
log_cb_orig could be assumed to be NULL and who cares about restoring the decor for ast_sip_destroy_system().
Line 155: struct pjsip_show_buildopts {
A better generic name is needed since there are going to be other CLI commands would be: pjsip_log_intercept
Actually with the suggestion in ast_sip_initialize_system() below eliminates the need for this struct and the show_buildopts global.
Line 158: const char *option;
: const char *format_string;
: va_list *arg_ptr;
: int sscanf_result;
These appear to be unused.
Line 173: logger =logger_cb;
Guidelines; spacing
Line 180: if (show_buildopts.fd == -1) {
Well, this isn't going to work to intercept when the output needs capturing and when it doesn't.
Line 201: #pragma GCC diagnostic ignored "-Wmissing-format-attribute"
Is this pragma needed since there is an __attribute__ associated with the prototype?
Line 216: for(i = 0; i < AST_VECTOR_SIZE(&buildopts); i++) {
guidelines: spacing
for () {
}
Line 217: res = vsscanf(AST_VECTOR_GET(&buildopts, i), format_temp, arg_ptr);
You need to do this for each loop iteration not once per function:
va_start()
vsscanf()
va_end()
Line 232: switch(cmd) {
spacing
switch () {
}
Line 246: for(i = 0; i < AST_VECTOR_SIZE(&buildopts); i++) {
spacing
Line 264: pj_log_set_log_func(log_cb);
How about before allowing any external logging routine to have a shot, you set the callback to an internal routine that only captures the buildopts output. Then change the callback to the one that allows external logging.
ast_sip_initialize_system() happens before any other thread operates with pjlib.
pj_log_set_log_func(capture_buildopts_cb)
pj_dump_config()
pj_log_set_log_func(log_cb)
Line 266: AST_VECTOR_INIT(&buildopts,64);
spacing
--
To view, visit https://gerrit.asterisk.org/2012
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icd23c289e1190edf9c6f07cabf382144ccef5b31
Gerrit-PatchSet: 5
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes
More information about the asterisk-code-review
mailing list