[Asterisk-code-review] res pjproject: Add module providing pjproject logging and u... (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: res_pjproject:  Add module providing pjproject logging and utils
......................................................................


Patch Set 9: Code-Review-1

(11 comments)

https://gerrit.asterisk.org/#/c/2012/9/res/res_pjproject.c
File res/res_pjproject.c:

Line 36:  <depend>pjproject</depend>
       :  <support_level>core</support_level>
unnecessary change from res_pjsip_log_forwarder

It might interfere with menuselect operation.


Line 81: 		/* For levels 3 and up, obey the debug level for res_pjsip */
       : 		mod_level = ast_opt_dbg_module ?
       : 											ast_debug_get_by_module("res_pjsip") :
       : 											0;
Should we care about publishing debug level if the module is res_pjsip or res_pjproject now?  I'm thinking it now should be res_pjproject.

Otherwise, unnecessary change from res_pjsip_log_forwarder


Line 105: 	return;
Useless return


Line 108: int ast_pjproject_get_buildopt(char *option, char *format_string, ...)
Since this function is exported for other modules to call there needs to be a way to prevent this module from being unloaded until all users have also been unloaded.  Look at res_stasis.c and the stasis_app_ref()/stasis_app_unref() calls.


Line 121: 		res = vsscanf(AST_VECTOR_GET(&buildopts, i), format_temp, arg_ptr);
I still think you need to have
va_start()
vsscanf()
va_end()

for each time through the loop as it is an assumption to think that vsscanf is not going to touch arg_ptr until the wanted line is found.


Line 138: 		e->command = "pjsip show buildopts";
Since we are moving res_pjsip_log_forwarder to be more accurately named res_pjproject, we should change this recently added CLI command to "pjproject show buildopts" to better reflect that chan_pjsip is not required.


Line 141: 				"       Show the compile time config of pjproject that Asterisk is\n"
        : 				"       running against.\n";
excess indention added for an already indented line continuation.


Line 154: 	return CLI_SUCCESS ;
semicolon spacing


Line 157: static struct ast_cli_entry pjsip_cli[] =
        : 	{
        : 			AST_CLI_DEFINE(handle_pjsip_show_buildopts, "Show the compiled config of pjproject in use"),
        : 		};
Did this file get put through indent or something?


Line 171: 	AST_VECTOR_INIT(&buildopts, 64);
AST_VECTOR_INIT() can fail to allocate the initial vector.


Line 205: AST_MODULE_INFO(ASTERISK_GPL_KEY, AST_MODFLAG_GLOBAL_SYMBOLS | AST_MODFLAG_LOAD_ORDER,
        : 	"PJPROJECT Log and Utility Support",
I think menuselect (or some another utility) requires this to be on one line.


-- 
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: 9
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: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes