[Asterisk-code-review] res pjproject: Add ability to map pjproject log levels to A... (asterisk[13])

[Kevin Harwell]

Kevin Harwell has posted comments on this change.

Change subject: res_pjproject:  Add ability to map pjproject log levels to Asterisk log levels
......................................................................


Patch Set 5:

(3 comments)

https://gerrit.asterisk.org/#/c/2245/5/res/res_pjproject.c
File res/res_pjproject.c:

Line 52: 					<note><para>The id of this object, as well as its type, must be
       : 					'log_mappings' or it won't be found.</para></note>
> Yep.  If we don't know the exact id, we have to search (retrieve_fields) so
Makes sense to me.


Line 149: 	RAII_VAR(struct log_mappings *, mappings, get_log_mappings(), ao2_cleanup);
        : 	unsigned char l;
        : 
        : 	if (!mappings) {
        : 		return __LOG_ERROR;
        : 	}
        : 
        : 	l = '0' + fmin(pj_level, 9);
        : 
        : 	if (strchr(mappings->asterisk_error, l)) {
        : 		return __LOG_ERROR;
        : 	} else if (strchr(mappings->asterisk_warning, l)) {
        : 		return __LOG_WARNING;
        : 	} else if (strchr(mappings->asterisk_notice, l)) {
        : 		return __LOG_NOTICE;
        : 	} else if (strchr(mappings->asterisk_verbose, l)) {
        : 		return __LOG_VERBOSE;
        : 	} else if (strchr(mappings->asterisk_debug, l)) {
        : 		return __LOG_DEBUG;
        : 	}
> Caching makes realtime difficult
Ah yes realtime. This makes sense as well. I don't foresee it being a problem either.


Line 379: 	ast_sorcery_object_field_register(pjproject_sorcery, "log_mappings", "type", "", OPT_NOOP_T, 0, 0);
        : 	ast_sorcery_object_field_register(pjproject_sorcery, "log_mappings", "asterisk_debug", "", OPT_STRINGFIELD_T, 0, STRFLDSET(struct log_mappings, asterisk_debug));
        : 	ast_sorcery_object_field_register(pjproject_sorcery, "log_mappings", "asterisk_error", "",  OPT_STRINGFIELD_T, 0, STRFLDSET(struct log_mappings, asterisk_error));
        : 	ast_sorcery_object_field_register(pjproject_sorcery, "log_mappings", "asterisk_warning", "",  OPT_STRINGFIELD_T, 0, STRFLDSET(struct log_mappings, asterisk_warning));
        : 	ast_sorcery_object_field_register(pjproject_sorcery, "log_mappings", "asterisk_notice", "",  OPT_STRINGFIELD_T, 0, STRFLDSET(struct log_mappings, asterisk_notice));
        : 	ast_sorcery_object_field_register(pjproject_sorcery, "log_mappings", "asteris
> Defaults only get applied if the file/section isn't found.  Otherwise a def
Okay I see what you are saying. It would be easy for someone to unwittingly have the same log level specified in two places. I'm fine leaving it as is (not adding some kind of conflict check or something), but perhaps a comment stating just what you said.


-- 
To view, visit https://gerrit.asterisk.org/2245
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba7bb349c70397586889b8f45b8c3d6c6c8c3898
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: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-HasComments: Yes