<p>Corey Farrell <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/6819">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 style="white-space: pre-wrap; word-wrap: break-word;">I raised this review on IRC, it was asked: why not just tail the cdr.csv file, transform, and put it into beanstalk? I have no opinion on this, just sharing a question that was asked.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I believe this is a comprehensive review of technical issues with the current patch. If you haven't already please take a look at the wiki [1] for instructions on updating this review (instead of abandoning and creating a new one).</p><p style="white-space: pre-wrap; word-wrap: break-word;">[1] https://wiki.asterisk.org/wiki/display/AST/Gerrit+Usage#GerritUsage-UpdatingaReview</p><p>(18 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/6819/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/6819/1//COMMIT_MSG@16">Patch Set #1, Line 16:</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;">At the same time, I cleaned up some minor syntax correction inside<br>cdr_manager.c, per remarks by Corey Farrel<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">No need to credit reviewers in the commit message.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c">File cdr/cdr_beanstalkd.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@22">Patch Set #1, Line 22:</a> <code style="font-family:monospace,monospace"> *</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Can we add a line referring to the source repository of the beanstalk client library this links against? I assume it's https://github.com/deepfryed/beanstalk-client? We'll need to know so we can verify the license is acceptable for linking in Asterisk.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@38">Patch Set #1, Line 38:</a> <code style="font-family:monospace,monospace"> <depend>beanstalk</depend></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Need to add a line to the MODULEINFO block:<br><support_level>extended</support_level></p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@42">Patch Set #1, Line 42:</a> <code style="font-family:monospace,monospace"> <managerEvent language="en_US" name="Cdr"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Sorry I didn't catch this before, this 'managerEvent' is only for cdr_manager. I don't think this module needs an xml documentation block.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@239">Patch Set #1, Line 239:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If someone has a config file with 'port=13001' but then removes the port setting and reloads, we probably want the port to be reset to default (13000) after the reload. This is the place to do that.</p><p style="white-space: pre-wrap; word-wrap: break-word;">This is also where we should set default allocated values for bs_host / bs_tube.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@245">Patch Set #1, Line 245:</a> <code style="font-family:monospace,monospace"> if (!newenablecdr) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If someone has config:<br>[general]<br>host=mybeanstalkd<br>enabled=true</p><p style="white-space: pre-wrap; word-wrap: break-word;">You will ignore the host setting because newenablecdr will not be true yet. If someone sets 'enabled=false' but still sets host, the value of bs_host needs to be set per user config even though enablecdr will be false.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@251">Patch Set #1, Line 251:</a> <code style="font-family:monospace,monospace"> bs_host = ast_strdup(v->value);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This leaks. We need to do a few things to deal with this leak.</p><p style="white-space: pre-wrap; word-wrap: break-word;">1) Set/reset allocated default values above the while loop.<br>2) When we get a new "host" or "tube" value we need to ast_free the existing one before setting a new one.<br>3) ast_free config values during module_unload.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@255">Patch Set #1, Line 255:</a> <code style="font-family:monospace,monospace"> bs_tube = ast_strdup(v->value);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Leak (comment for bs_host leak).</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@258">Patch Set #1, Line 258:</a> <code style="font-family:monospace,monospace"> }</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Did we lose the option for setting customfields?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@263">Patch Set #1, Line 263:</a> <code style="font-family:monospace,monospace"> ast_log(LOG_NOTICE, "Added beanstalkd server %s at port %d with tube %s", bs_host, bs_port, bs_tube);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This seems out of place. Probably better to put it where we call ast_cdr_backend_unsuspend. I'd suggest instead of "Added" say "Enabled".</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@297">Patch Set #1, Line 297:</a> <code style="font-family:monospace,monospace"> bs_socket = bs_connect(bs_host, bs_port);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The use of bs_host (and bs_tube just below) need to be protected by customfields_lock. That lock would be more appropriately named 'config_lock' since it needs to prevent use after free for the configured strings.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@316">Patch Set #1, Line 316:</a> <code style="font-family:monospace,monospace"> //cdr_buffer[0] = '\0';</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Just delete this line, don't comment out.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@352">Patch Set #1, Line 352:</a> <code style="font-family:monospace,monospace"> cdr_buffer = ast_json_dump_string(t_cdr_json);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You need to ast_json_unref(t_cdr_json) after you are done with it.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@354">Patch Set #1, Line 354:</a> <code style="font-family:monospace,monospace"> /*</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Delete this code instead of commenting out.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@376">Patch Set #1, Line 376:</a> <code style="font-family:monospace,monospace"> bs_disconnect(bs_socket);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You need a call to ast_json_free(cdr_buffer) now that you're done with the value or this will be a major memory leak.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@385">Patch Set #1, Line 385:</a> <code style="font-family:monospace,monospace"> if (customfields) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ast_free is NULL safe so no need to check if it's NULL. This would be the place to free bs_host and bs_tube as well.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_beanstalkd.c@410">Patch Set #1, Line 410:</a> <code style="font-family:monospace,monospace"> .support_level = AST_MODULE_SUPPORT_CORE,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This module will be AST_MODULE_SUPPORT_EXTENDED.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_manager.c">File cdr/cdr_manager.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6819/1/cdr/cdr_manager.c">Patch Set #1:</a> </p><p style="white-space: pre-wrap; word-wrap: break-word;">Although I have no objection to these changes they are unrelated and should not be included in the commit with your new module.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/6819">change 6819</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/6819"/><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: I5fe4089a34ab3b39230786d9bbfddafa56715f48 </div>
<div style="display:none"> Gerrit-Change-Number: 6819 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Nir Simionovich (GreenfieldTech - Israel) <nirs@greenfieldtech.net> </div>
<div style="display:none"> Gerrit-Reviewer: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 17 Oct 2017 17:38:32 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>