<p> Attention is currently required from: Joshua Colp, George Joseph, Kevin Harwell. </p>
<p><a href="https://gerrit.asterisk.org/c/asterisk/+/16222">View Change</a></p><p>5 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">File main/logger.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16222/comment/60b61da7_9035b083">Patch Set #12, Line 831:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">int custom_level, found = 0;<br> /* if we are reloading, we don't want to re-register levels that exist */<br> for (level = 16; level < ARRAY_LEN(levels); level++) {<br> if (levels[level] && !strcasecmp(levels[level], logfile)) {<br> found = 1; /* already exists, skip */<br> break;<br> }<br> }<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I believe you can just call 'ast_logger_get_dynamic_level' here?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Not quite, because that results in a deadlock, but I added a logger_get_dynamic_level which allows the simplification.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16222/comment/83b26bf6_fa7cfb69">Patch Set #12, Line 847:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> for (level = 16; level < ARRAY_LEN(levels); level++) {<br> if (levels[level] && custom_dynamic_levels[level]) {<br> int level2, found = 0;<br> for (level2 = 16; level2 < ARRAY_LEN(levels); level2++) {<br> if (levels[level2] && !strcasecmp(levels[level2], levels[level])) {<br> found = 1; /* level still exists, skip */<br> break;<br> }<br> }<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This might could be simplified if you instead loop over the custom_dynamic_levels, and then you can […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Not quite sure I follow... can you elaborate on this?<br>I'm able to simplify it quite a bit if I do it before, but not sure how to take that further.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16222/comment/69d7c486_b020cf34">Patch Set #12, Line 846:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /* unregister existing custom levels which don't exist anymore */<br> for (level = 16; level < ARRAY_LEN(levels); level++) {<br> if (levels[level] && custom_dynamic_levels[level]) {<br> int level2, found = 0;<br> for (level2 = 16; level2 < ARRAY_LEN(levels); level2++) {<br> if (levels[level2] && !strcasecmp(levels[level2], levels[level])) {<br> found = 1; /* level still exists, skip */<br> break;<br> }<br> }<br> if (!found) {<br> logger_unregister_level(levels[level]);<br> custom_dynamic_levels[level] = 0;<br> }<br> }<br> }<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Probably want to remove the deleted items prior to [re]loading new ones as removing will free up pos […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16222/comment/17e71557_e8e3ea44">Patch Set #12, Line 2604:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> unsigned int found = 0;<br> unsigned int x;<br><br> for (x = 16; x < ARRAY_LEN(levels); x++) {<br> if (!levels[x]) {<br> continue;<br> }<br><br> if (strcasecmp(levels[x], name)) {<br> continue;<br> }<br><br> found = 1;<br> break;<br> }<br><br> if (found) {<br> /* take this level out of the global_logmask, to ensure that no new log messages<br> * will be queued for it<br> */<br><br> global_logmask &= ~(1 << x);<br><br> ast_free(levels[x]);<br> levels[x] = NULL;<br> return x;<br> }<br><br> return 0;<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Could this whole method be reduced to: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16222/comment/0ba29035_13a5b3a5">Patch Set #12, Line 2637:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> int x;<br><br> AST_RWLIST_WRLOCK(&logchannels);<br> x = logger_unregister_level(name);<br><br> if (x) {<br> AST_RWLIST_UNLOCK(&logchannels);<br><br> ast_debug(1, "Unregistered dynamic logger level '%s' with index %u.\n", name, x);<br><br> AST_RWLIST_WRLOCK(&logchannels);<br> update_logchannels();<br> AST_RWLIST_UNLOCK(&logchannels);<br> } else {<br> AST_RWLIST_UNLOCK(&logchannels);<br> }<br>}<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Seems odd to just have to unlock for the debug message. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/16222">change 16222</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/16222"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: If082703cf81a436ae5a565c75225fa8c0554b702 </div>
<div style="display:none"> Gerrit-Change-Number: 16222 </div>
<div style="display:none"> Gerrit-PatchSet: 14 </div>
<div style="display:none"> Gerrit-Owner: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-Attention: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Attention: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Attention: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 13 Sep 2021 10:35:22 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>