[Asterisk-code-review] logger: Add custom logging capabilities (asterisk[master])

[Kevin Harwell]

Attention is currently required from: N A, Joshua Colp.
Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/16222 )

Change subject: logger: Add custom logging capabilities
......................................................................


Patch Set 6: Code-Review-1

(3 comments)

File apps/app_verbose.c:

https://gerrit.asterisk.org/c/asterisk/+/16222/comment/9c7802ec_800c01d7 
PS6, Line 139: 	} else {
             : 		int level = ast_logger_dynamic_level(args.level);
             : 		if (level > -1) {
             : 			ast_log_dynamic_level(level, "%s\n", args.msg);
             : 		} else {
             : 			ast_log(LOG_ERROR, "Unknown log level: '%s'\n", args.level);
             : 		}
             : 	}
             : 
             : 	if (lnum > -1) {
             : 		snprintf(context, sizeof(context), "@ %s", ast_channel_context(chan));
             : 		snprintf(extension, sizeof(extension), "Ext. %s", ast_channel_exten(chan));
             : 
             : 		ast_log(lnum, extension, ast_channel_priority(chan), context, "%s\n", args.msg);
             : 	}
Since this is for logging custom levels via the dialplan I think you'd want to log the extension, priority, and context like the other levels do. As such you can simplify this by setting lnum = ast_logger_dynamic_level(args.level) in the 'else' statement, and then moving the (LOG_ERROR, "Unknown ....") to an 'else' off of the 'if (lnum > -1)'


File configs/samples/logger.conf.sample:

https://gerrit.asterisk.org/c/asterisk/+/16222/comment/c5c35ab0_2ad8d634 
PS6, Line 92: ;custom_logs
I'm not a fan of custom_logs. What about dynamic_levels, custom_levels, user_levels or something else?


File include/asterisk/logger.h:

https://gerrit.asterisk.org/c/asterisk/+/16222/comment/2680c35b_a78dd58d 
PS6, Line 333:  * \brief Checks if a dynamic logger level exists
             :  * \param name The name of the dynamic level for which to check existence
             :  * \retval -1 if no such dynamic level exists
             :  * \retval positive log level to be used with ast_log_dynamic_level for sending messages to this level
             :  */
While this does "check" to me it's more of a retrieval. How about something like the following instead?

\brief "Retrieve dynamic logging level id"
\param name The name of the level
\retval The unique integer id for the given level
\retval -1 if level name not found



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/16222
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: If082703cf81a436ae5a565c75225fa8c0554b702
Gerrit-Change-Number: 16222
Gerrit-PatchSet: 6
Gerrit-Owner: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Attention: N A <mail at interlinked.x10host.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Wed, 11 Aug 2021 21:52:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210811/7f72294f/attachment.html>