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

(2 comments)

File main/logger.c:

https://gerrit.asterisk.org/c/asterisk/+/16222/comment/57aa53da_aab9bae5 
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;
              : 					}
              : 				}
> Not quite sure I follow... can you elaborate on this? […]
Your new patch is better, but I'll follow with a comment there and talk about what I meant.


File main/logger.c:

https://gerrit.asterisk.org/c/asterisk/+/16222/comment/bd112660_ab8b89e7 
PS14, Line 832: 		for (level = 16; level < ARRAY_LEN(levels); level++) {
              : 			if (levels[level] && custom_dynamic_levels[level]) {
              : 				logger_unregister_level(levels[level]);
              : 				custom_dynamic_levels[level] = 0;
              : 			}
              : 		}
If I understand this bit of code correctly then all currently registered custom dynamic levels get unregistered, which then they possibly get re-registered again below. This might be fine to do, but

1) Would it be possible for a thread to get a level, the level changes for that custom value, and then write to previously retrieved level? Thus potentially writing to the wrong level.

2) Similar to the above, but no level may be available for a short time. Meaning could all custom levels be removed, and then another thread ask for one and get "no  matching custom level available"? I think this second case is protected by current locks though?

Anyway if either of those are problems, or you just wanted to do it differently such that you only removed custom levels that are no longer configured then I was thinking of something like the following (continuation of a previous comment):

Instead of looping over the level array here, you'd want to loop over the custom_dynamic_levels array, and see if it matched with any in the incoming "customlogs" (a 'strstr' call might be sufficient?). If so do nothing/keep it. If no match then remove from custome_dynamic_levels array and unregister from levels.



-- 
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: 14
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-Comment-Date: Wed, 15 Sep 2021 16:12:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: N A <mail at interlinked.x10host.com>
Comment-In-Reply-To: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210915/ca3fd4a5/attachment.html>