<p> Attention is currently required from: N A, Joshua Colp, George Joseph. </p>
<p>Patch set 12:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></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/bbefab62_d72f8949">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 style="white-space: pre-wrap; word-wrap: break-word;">I believe you can just call 'ast_logger_get_dynamic_level' here?</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/b0cdae60_140cb0b2">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 style="white-space: pre-wrap; word-wrap: break-word;">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'.</p><p style="white-space: pre-wrap; word-wrap: break-word;">This of course would only work if you moved removal to be prior to adding. See next comment below for more info.</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/780161fe_72c0921c">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 style="white-space: pre-wrap; word-wrap: break-word;">Probably want to remove the deleted items prior to [re]loading new ones as removing will free up possibly needed space.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</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/a8d84cda_2483cc22">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 style="white-space: pre-wrap; word-wrap: break-word;">Could this whole method be reduced to:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">x = ast_logger_get_dynamic_level(name);<br>if (x == -1) {<br> return 0;<br>}<br>global_logmask ....<br>astfree ....<br>levels[x] = NULL<br>return x;</pre><p style="white-space: pre-wrap; word-wrap: break-word;">?</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/7f0d0ee6_a3bd12ff">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 style="white-space: pre-wrap; word-wrap: break-word;">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.</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: 12 </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: 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-Comment-Date: Fri, 10 Sep 2021 21:31:08 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>