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

[Kevin Harwell]

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

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


Patch Set 12: Code-Review-1

(5 comments)

File main/logger.c:

https://gerrit.asterisk.org/c/asterisk/+/16222/comment/bbefab62_d72f8949 
PS12, Line 831: int custom_level, found = 0;
              : 			/* if we are reloading, we don't want to re-register levels that exist */
              : 			for (level = 16; level < ARRAY_LEN(levels); level++) {
              : 				if (levels[level] && !strcasecmp(levels[level], logfile)) {
              : 					found = 1; /* already exists, skip */
              : 					break;
              : 				}
              : 			}
I believe you can just call 'ast_logger_get_dynamic_level' here?


https://gerrit.asterisk.org/c/asterisk/+/16222/comment/b0cdae60_140cb0b2 
PS12, Line 847: 		for (level = 16; level < ARRAY_LEN(levels); level++) {
              : 			if (levels[level] && custom_dynamic_levels[level]) {
              : 				int level2, found = 0;
              : 				for (level2 = 16; level2 < ARRAY_LEN(levels); level2++) {
              : 					if (levels[level2] && !strcasecmp(levels[level2], levels[level])) {
              : 						found = 1; /* level still exists, skip */
              : 						break;
              : 					}
              : 				}
This might could be simplified if you instead loop over the custom_dynamic_levels, and then you can just call 'ast_logger_get_dynamic_level'.

This of course would only work if you moved removal to be prior to adding. See next comment below for more info.


https://gerrit.asterisk.org/c/asterisk/+/16222/comment/780161fe_72c0921c 
PS12, Line 846: 		/* unregister existing custom levels which don't exist anymore */
              : 		for (level = 16; level < ARRAY_LEN(levels); level++) {
              : 			if (levels[level] && custom_dynamic_levels[level]) {
              : 				int level2, found = 0;
              : 				for (level2 = 16; level2 < ARRAY_LEN(levels); level2++) {
              : 					if (levels[level2] && !strcasecmp(levels[level2], levels[level])) {
              : 						found = 1; /* level still exists, skip */
              : 						break;
              : 					}
              : 				}
              : 				if (!found) {
              : 					logger_unregister_level(levels[level]);
              : 					custom_dynamic_levels[level] = 0;
              : 				}
              : 			}
              : 		}
Probably want to remove the deleted items prior to [re]loading new ones as removing will free up possibly needed space.

For instance say you already have 16 custom levels, on reload you remove 1. Currently as is this will claim there are no levels left to add, and then remove the 1. While deleting first will free up the extra one spot you need.


https://gerrit.asterisk.org/c/asterisk/+/16222/comment/a8d84cda_2483cc22 
PS12, Line 2604: 	unsigned int found = 0;
               : 	unsigned int x;
               : 
               : 	for (x = 16; x < ARRAY_LEN(levels); x++) {
               : 		if (!levels[x]) {
               : 			continue;
               : 		}
               : 
               : 		if (strcasecmp(levels[x], name)) {
               : 			continue;
               : 		}
               : 
               : 		found = 1;
               : 		break;
               : 	}
               : 
               : 	if (found) {
               : 		/* take this level out of the global_logmask, to ensure that no new log messages
               : 		 * will be queued for it
               : 		 */
               : 
               : 		global_logmask &= ~(1 << x);
               : 
               : 		ast_free(levels[x]);
               : 		levels[x] = NULL;
               : 		return x;
               : 	}
               : 
               : 	return 0;
Could this whole method be reduced to:

x = ast_logger_get_dynamic_level(name);
if (x == -1) {
  return 0;
}
global_logmask ....
astfree ....
levels[x] = NULL
return x;

?


https://gerrit.asterisk.org/c/asterisk/+/16222/comment/7f0d0ee6_a3bd12ff 
PS12, Line 2637: 	int x;
               : 
               : 	AST_RWLIST_WRLOCK(&logchannels);
               : 	x = logger_unregister_level(name);
               : 
               : 	if (x) {
               : 		AST_RWLIST_UNLOCK(&logchannels);
               : 
               : 		ast_debug(1, "Unregistered dynamic logger level '%s' with index %u.\n", name, x);
               : 
               : 		AST_RWLIST_WRLOCK(&logchannels);
               : 		update_logchannels();
               : 		AST_RWLIST_UNLOCK(&logchannels);
               : 	} else {
               : 		AST_RWLIST_UNLOCK(&logchannels);
               : 	}
               : }
Seems odd to just have to unlock for the debug message. I think this could be refactored to reduce the locking/unlocking. Could prob even drop the 'else' as well.



-- 
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: 12
Gerrit-Owner: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: N A <mail at interlinked.x10host.com>
Gerrit-Attention: N A <mail at interlinked.x10host.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: George Joseph <gjoseph at digium.com>
Gerrit-Comment-Date: Fri, 10 Sep 2021 21:31:08 +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/20210910/87b1b277/attachment-0001.html>