<p>Kevin Harwell <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/7928">View Change</a></p><p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p>(4 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/7928/1//COMMIT_MSG">Commit Message:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7928/1//COMMIT_MSG@12">Patch Set #1, Line 12:</a> <code style="font-family:monospace,monospace">However it dos not run the full connection startup sequence and thus</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/dos/does</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/7928/1/addons/cdr_mysql.c">File addons/cdr_mysql.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7928/1/addons/cdr_mysql.c@176">Patch Set #1, Line 176:</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;"> if (!((hostname || dbsock) && dbuser && password && dbname<br> && dbtable) ) <br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">If I am reading the code right these values will always be non-null (unless an allocation failed on load). Probably the intention was to check if the ast_str actually had a value.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also if these values are required for connection then it's probably better to check right after the configuration has been loaded. Maybe even rejecting the configuration (module decline on load, or keep old config in reload case).</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7928/1/addons/cdr_mysql.c@188">Patch Set #1, Line 188:</a> <code style="font-family:monospace,monospace"> struct ast_config *cfg = ast_config_load(config, config_flags);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You are reloading the config here, but not updating the global configuration variables. I think either you can just call the module's reload here or if those values don't need to be set then not do a config load here.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7928/1/addons/cdr_mysql.c@622">Patch Set #1, Line 622:</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;"> res |= my_load_config_string(cfg, "global", "hostname", &hostname, "localhost");<br> res |= my_load_config_string(cfg, "global", "dbname", &dbname, "astriskcdrdb");<br> res |= my_load_config_string(cfg, "global", "user", &dbuser, "root");<br> res |= my_load_config_string(cfg, "global", "sock", &dbsock, "");<br> res |= my_load_config_string(cfg, "global", "table", &dbtable, "cdr");<br> res |= my_load_config_string(cfg, "global", "password", &password, "");<br><br> res |= my_load_config_string(cfg, "global", "charset", &dbcharset, "");<br><br> res |= my_load_config_string(cfg, "global", "ssl_ca", &ssl_ca, "");<br> res |= my_load_config_string(cfg, "global", "ssl_cert", &ssl_cert, "");<br> res |= my_load_config_string(cfg, "global", "ssl_key", &ssl_key, "");<br><br> res |= my_load_config_number(cfg, "global", "port", &dbport, MYSQL_PORT);<br> res |= my_load_config_number(cfg, "global", "timeout", &timeout, 0);<br> res |= my_load_config_string(cfg, "global", "compat", &compat, "no");<br> res |= my_load_config_string(cfg, "global", "cdrzone", &cdrzone, "");<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">If you do end up keeping the config reload stuff and you call this on reconnect then I think these ast_str global variables are being leaked.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/7928">change 7928</a>. To unsubscribe, 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/7928"/><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-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I7603c7501dae7070fac35081cf35161579c47590 </div>
<div style="display:none"> Gerrit-Change-Number: 7928 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Tzafrir Cohen <tzafrir.cohen@xorcom.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Tzafrir Cohen <tzafrir.cohen@xorcom.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 12 Jan 2018 23:38:01 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>