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

[George Joseph]

George Joseph has posted comments on this change.

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


Patch Set 9:

(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
Done


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 r
Done


Line 105: 	return;
> Useless return
Done


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
Done


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


Line 138: 		e->command = "pjsip show buildopts";
> Since we are moving res_pjsip_log_forwarder to be more accurately named res
Done


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.
Done


Line 154: 	return CLI_SUCCESS ;
> semicolon spacing
Done


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?
I think my format-on-save is messed up again.


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


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 lin
Done


-- 
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