[Asterisk-code-review] res pjsip/config system: Add ast sip get pjproject buildopt (asterisk[13])

[Richard Mudgett]

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