<p> Attention is currently required from: Sean Bright, Dennis. </p>
<p>Patch set 3:<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/+/14698">View Change</a></p><p>8 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">Commit Message:</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/+/14698/comment/5b40876e_744c9802">Patch Set #3, Line 57:</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;">Hopefully I was able to re-implement the "static", "filter" and "alias"<br>features properly. Documentation on how they actually function was<br>somewhat light.<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">As such I'd recommend adding more documentation to the cel_odbc.conf.sample file describing how these parameters at least now work (post your patch that is).</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File cel/cel_odbc.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/+/14698/comment/4c8f751f_bfb670f9">Patch Set #3, Line 581:</a> <code style="font-family:monospace,monospace">ast_strdupa</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Stack allocation like this should really never be done in a loop as it increases the potential of blowing the stack.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I do realize this was part of the pre-patched code. As a check is done prior and if it can be guaranteed to only be done a limited number of times then this _might_ be okay.</p><p style="white-space: pre-wrap; word-wrap: break-word;">However, I think since modifying the code anyways, and given that there could be 'N' aliases I lean toward having this handled in a different way.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14698/comment/03089e58_6eac69f0">Patch Set #3, Line 590:</a> <code style="font-family:monospace,monospace"> char *value = strip_quotes(ast_strip(ast_strdupa(var->name + 6)));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I believe there can be an unknown number of static values as well? If so then this stack allocation in a loop also needs removal/changing.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14698/comment/d5b1b797_5d1b1339">Patch Set #3, Line 596:</a> <code style="font-family:monospace,monospace"> char *celname = ast_strip(ast_strdupa(var->name + 6));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Another stack allocation case to consider changing.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14698/comment/b62e3f69_fde67944">Patch Set #3, Line 722:</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;">static int start_table_manager(void)<br>{<br> int errcode = 0;<br><br> ast_sem_init(&manager_sem, 0, 0);<br><br> if ((errcode = ast_pthread_create(&manager_thread, NULL, mb_manager_thread, NULL))) {<br> ast_log(LOG_ERROR, "Could not create thread: %s\n", strerror(errcode));<br> return 0;<br> } else {<br> return 1;<br> }<br>}<br><br>static void stop_table_manager(void)<br>{<br> ast_sem_post(&manager_sem);<br> pthread_join(manager_thread, NULL);<br> manager_thread = AST_PTHREADT_NULL;<br> ast_sem_destroy(&manager_sem);<br>}<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't quite understand the need for this table manager thread. Could you explain more/add a comment about what it does and why it's needed?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14698/comment/c05c9db3_9fec4378">Patch Set #3, Line 767:</a> <code style="font-family:monospace,monospace">SQLBindParameter</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Is the return value worth checking for success/failure?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Same for any other instances of this call below.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14698/comment/059e7889_585a9e41">Patch Set #3, Line 768:</a> <code style="font-family:monospace,monospace">SQL_TYPE_TIMESTAMP</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Are other time types also deduced here? For example, SQL_DATETIME? Just want to make sure we don't reintroduce something like this bug:</p><p style="white-space: pre-wrap; word-wrap: break-word;">See https://issues.asterisk.org/jira/browse/ASTERISK-30096</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14698/comment/e8c1ca53_48874af9">Patch Set #3, Line 993:</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;"> destroy_tables_safely();<br> load_config();<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think this will require locking on odbc_tables.</p><p style="white-space: pre-wrap; word-wrap: break-word;">While the tables are destroyed safely, it does not appear they are loaded safely. For instance what would happen if during a reload an attempt is made to write to odbc_tables while load_config was only partly done?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/14698">change 14698</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/+/14698"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 16 </div>
<div style="display:none"> Gerrit-Change-Id: Id85d81add33096f8282d212daf239f2fc845d783 </div>
<div style="display:none"> Gerrit-Change-Number: 14698 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Dennis <dennis.buteyn@xorcom.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Sean Bright <sean@seanbright.com> </div>
<div style="display:none"> Gerrit-CC: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Attention: Sean Bright <sean@seanbright.com> </div>
<div style="display:none"> Gerrit-Attention: Dennis <dennis.buteyn@xorcom.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 15 Jul 2022 18:34:54 +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>