<p>Corey Farrell <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/6841">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;">Caught another error when checking the CEL review.</p><p>(12 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/6841/1/cel/cel_beanstalkd.c">File cel/cel_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/6841/1/cel/cel_beanstalkd.c@4">Patch Set #1, Line 4:</a> <code style="font-family:monospace,monospace"> * Copyright (C) 2008 - 2009, Digium, Inc.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">2017, copyright is yours.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6841/1/cel/cel_beanstalkd.c@32">Patch Set #1, Line 32:</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;"> * \arg \ref AstCDR<br> * \arg \ref AstAMI<br> * \arg \ref Config_ami<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Probably AstCEL instead of these 3.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6841/1/cel/cel_beanstalkd.c@97">Patch Set #1, Line 97:</a> <code style="font-family:monospace,monospace"> char start_time[80] = "";</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">No need to initialize since it's unconditionally set.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6841/1/cel/cel_beanstalkd.c@183">Patch Set #1, Line 183:</a> <code style="font-family:monospace,monospace"> ast_log(LOG_WARNING, "Configuration file '%s' is invalid. CEL manager Module not activated.\n",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Not "CEL manager Module"</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6841/1/cel/cel_beanstalkd.c@185">Patch Set #1, Line 185:</a> <code style="font-family:monospace,monospace"> enablecel = 0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If you want the behavior to be consistent with cdr_beanstalkd then you should not set enablecel=0 here, just leave the previous configuration if new config file is broken. That or make cdr_beanstalkd disable itself on invalid config.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6841/1/cel/cel_beanstalkd.c@188">Patch Set #1, Line 188:</a> <code style="font-family:monospace,monospace"> ast_log(LOG_WARNING, "Failed to load configuration file. CEL manager Module not activated.\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ditto.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6841/1/cel/cel_beanstalkd.c@189">Patch Set #1, Line 189:</a> <code style="font-family:monospace,monospace"> enablecel = 0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Before this you should ast_cel_backend_unregister if enablecel was previously set.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6841/1/cel/cel_beanstalkd.c@196">Patch Set #1, Line 196:</a> <code style="font-family:monospace,monospace"> ast_rwlock_wrlock(&config_lock);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Need to lock before freeing the values.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6841/1/cel/cel_beanstalkd.c@254">Patch Set #1, Line 254:</a> <code style="font-family:monospace,monospace"> ast_rwlock_wrlock(&config_lock);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">No locking here.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6841/1/cel/cel_beanstalkd.c@255">Patch Set #1, Line 255:</a> <code style="font-family:monospace,monospace"> ast_cel_backend_unregister(CEL_BACKEND_NAME);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Unregister the cel backend before freeing bs_host / bs_tube.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6841/1/cel/cel_beanstalkd.c@270">Patch Set #1, Line 270:</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;"> ast_free(bs_host);<br> ast_free(bs_tube);<br> ast_rwlock_wrlock(&config_lock);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should not be done here.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/6841/1/main/cel.c">File main/cel.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6841/1/main/cel.c@253">Patch Set #1, Line 253:</a> <code style="font-family:monospace,monospace"> .skip_category = "(^manager$|^radius$|^beanstalk$)", /*!< Config sections used by existing modules. Do not add to this list. */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">New CEL modules are expected to have their own config files. cel_custom does this instead of using the main cel.conf.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/6841">change 6841</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/6841"/><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: Ic3a087faeeac045d69a2a018e60e29831ddb95ab </div>
<div style="display:none"> Gerrit-Change-Number: 6841 </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: Fri, 20 Oct 2017 05:23:09 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>