[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 5:

(13 comments)

See 2 questions...

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)))
Actually it needs to be scanf as well.


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
Done


Line 155: struct pjsip_show_buildopts {
> A better generic name is needed since there are going to be other CLI comma
Done


Line 158: 	const char *option;
        : 	const char *format_string;
        : 	va_list *arg_ptr;
        : 	int sscanf_result;
> These appear to be unused.
Actually the whole structure isn't needed at this stage.


Line 173: 	logger =logger_cb;
> Guidelines; spacing
Done


Line 180: 	if (show_buildopts.fd == -1) {
> Well, this isn't going to work to intercept when the output needs capturing
It does work.  What's the issue?


Line 201: #pragma GCC diagnostic ignored "-Wmissing-format-attribute"
> Is this pragma needed since there is an __attribute__ associated with the p
Actually since i corrected the attribute from printf to scanf, no. :)


Line 216: 	for(i = 0; i < AST_VECTOR_SIZE(&buildopts); i++) {
> guidelines: spacing
Done


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:
Why, the arg_ptr isn't changing and only 1 vector entry will match.


Line 232: 	switch(cmd) {
> spacing
Done


Line 246: 	for(i = 0; i < AST_VECTOR_SIZE(&buildopts); i++) {
> spacing
Done


Line 264: 	pj_log_set_log_func(log_cb);
> How about before allowing any external logging routine to have a shot, you 
Done


Line 266: 	AST_VECTOR_INIT(&buildopts,64);
> spacing
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: 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: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes