<p><a href="https://gerrit.asterisk.org/c/asterisk/+/14698">View Change</a></p><p>9 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/f55adbf9_a93b198b">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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">As such I'd recommend adding more documentation to the cel_odbc.conf. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Updated cel_odbc.conf.sample documentation.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">Patchset:</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?tab=comments">Patch Set #4:</a> </p><p style="white-space: pre-wrap; word-wrap: break-word;">Made requested changes, also included ast_sem_timedwait fix mentioned previously. Pushed changes for Asterisk 18 (16 is now security fix only).</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/c7f24481_1790dbad">Patch Set #3, Line 581:</a> <code style="font-family:monospace,monospace">ast_strdupa</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Stack allocation like this should really never be done in a loop as it increases the potential of bl […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Stack allocation has been moved into a new function, preventing stack overflow.</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/c8e32769_7085fd3b">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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I believe there can be an unknown number of static values as well? If so then this stack allocation […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Stack allocation has been moved into a new function, preventing stack overflow.</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/83fd2cdd_e7d1579d">Patch Set #3, Line 596:</a> <code style="font-family:monospace,monospace"> char *celname = ast_strip(ast_strdupa(var->name + 6));</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Another stack allocation case to consider changing.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Stack allocation has been moved into a new function, preventing stack overflow.</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/785dcd3a_89bc51fd">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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I don't quite understand the need for this table manager thread. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">In order to construct all the insert queries, table information needs to be queried. This can be a lengthy process, possibly taking forever when one or more databases remain unavailable. Blocking load_module for that long is a bit unreasonable so this manager does all the work instead.</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/cfe7d4ce_3b32c535">Patch Set #3, Line 767:</a> <code style="font-family:monospace,monospace">SQLBindParameter</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Is the return value worth checking for success/failure? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">SQLBindParameter can fail, yes. I've added checks for SQLBindParameter, SQLGetData, SQLGetDiagField, SQLGetDiagRec and SQLRowCount.</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/36da5470_3bbd0c78">Patch Set #3, Line 768:</a> <code style="font-family:monospace,monospace">SQL_TYPE_TIMESTAMP</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Are other time types also deduced here? For example, SQL_DATETIME? Just want to make sure we don't r […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">There is no type deduction going on here. The original version of cel_odbc constructed queries with embedded, raw values and required type deduction for figuring out how to format the values as strings. This rewritten version uses parameterized queries which moves the responsibility of type conversion to the database. Aside from eliminating SQL injections, parameterized queries also remove the need to construct a brand new query for every single insert.</p><p style="white-space: pre-wrap; word-wrap: break-word;">For this specific call to SQLBindParameter, SQL_TYPE_TIMESTAMP defines the type (a timestamp) of the value event_time as it is bound as the index'd parameter.</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/235ef7d8_8a02990c">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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think this will require locking on odbc_tables. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Wrapped a write lock around load_config.</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: 4 </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 <default.enum@gmail.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-Comment-Date: Sun, 06 Nov 2022 12:12:08 +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 <default.enum@gmail.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>