<p> Attention is currently required from: Sean Bright, Joshua Colp, Kevin Harwell. </p>
<p><a href="https://gerrit.asterisk.org/c/asterisk/+/17792">View Change</a></p><p>12 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">File include/asterisk/chanvars.h:</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/+/17792/comment/064cc5a9_16934b05">Patch Set #1, Line 28:</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;">/*!<br> * \brief special variable types<br> * \note These only apply to special Asterisk variables (i.e. all caps)<br> */<br>enum ast_variable_type {<br> AST_VAR_SYSTEM, /*!< Special variables that pertain to the entire Asterisk system (R/O) */<br> AST_VAR_CHANNEL_BUILTIN, /*!< Special variables that pertain to a specific channel (R/O) */<br> AST_VAR_MODULE, /*!< Special variables used by individual modules (R/O) */<br> AST_VAR_MODULE_WRITE, /*!< Special variables used by individual modules, that a user may write */<br>};<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">See a later comment about creating a new variable structure, but if you do do something like that th […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17792/comment/002e88af_9114186c">Patch Set #1, Line 53:</a> <code style="font-family:monospace,monospace">int ast_varname_reserved(const char *name);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Conventionally this should be named *_is_reserved.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17792/comment/5ac4e583_162b4133">Patch Set #1, Line 63:</a> <code style="font-family:monospace,monospace">int ast_variable_editable(const char *name);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Same as above, conventionally this should be named *_is_editable. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17792/comment/d8a6e180_d7f5a824">Patch Set #1, Line 77:</a> <code style="font-family:monospace,monospace">int ast_bad_variable_assignment(const char *name);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This should start with "ast_variable_". […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File main/pbx_variables.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/+/17792/comment/2979aa9a_458638ea">Patch Set #1, Line 112:</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_RWLOCK_DEFINE_STATIC(systemvarslock);<br>AST_RWLOCK_DEFINE_STATIC(specialchanvarslock);<br>AST_RWLOCK_DEFINE_STATIC(modvarslock);<br>AST_RWLOCK_DEFINE_STATIC(modeditvarslock);<br>static struct varshead systemvars = AST_LIST_HEAD_NOLOCK_INIT_VALUE;<br>static struct varshead specialchanvars = AST_LIST_HEAD_NOLOCK_INIT_VALUE;<br>static struct varshead modvars = AST_LIST_HEAD_NOLOCK_INIT_VALUE;<br>static struct varshead modeditvars = AST_LIST_HEAD_NOLOCK_INIT_VALUE;<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">At a glance I do not see anything different about it, or see why it too could not be refactored to u […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17792/comment/0dc76299_14794a7c">Patch Set #1, Line 116:</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 struct varshead systemvars = AST_LIST_HEAD_NOLOCK_INIT_VALUE;<br>static struct varshead specialchanvars = AST_LIST_HEAD_NOLOCK_INIT_VALUE;<br>static struct varshead modvars = AST_LIST_HEAD_NOLOCK_INIT_VALUE;<br>static struct varshead modeditvars = AST_LIST_HEAD_NOLOCK_INIT_VALUE;<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I'll look into the vector approach. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17792/comment/2e30ebc1_9eac4d70">Patch Set #1, Line 134:</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 (!add && value) { /* delete an existing var */<br> ast_debug(1, "Deregistering special variable '%s'\n", name);<br> AST_LIST_REMOVE_CURRENT(entries);<br> ast_var_delete(newvariable);<br> break;<br> }<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think de-registering should be in separate function.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17792/comment/3dd9f263_17dec8ef">Patch Set #1, Line 144:</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 (res && value) {<br> ast_log(LOG_WARNING, "Failed to catch duplicate registration of '%s'\n", name); /* should never happen */<br> return -1;<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">From above it appears the only way res != 0 is if !value. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17792/comment/498a8a83_99e3660f">Patch Set #1, Line 155:</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;">int ast_varname_reserved(const char *name)<br>{<br> char *tmp = (char*) name;<br> /* Asterisk broadly reserves all uppercase + underscore var names for use by Asterisk */<br> while (*tmp) {<br> if (tmp[0] != '_' && !isupper(tmp[0])) {<br> return 0;<br> }<br> tmp++;<br> }<br> return 1;<br>}<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">> Au contraire, that *has* been the longstanding precedent […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17792/comment/7ed7ee77_7f64546a">Patch Set #1, Line 177:</a> <code style="font-family:monospace,monospace"> ast_rwlock_wrlock(&modeditvarslock);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This can be a read lock.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17792/comment/51736f53_fd3dd307">Patch Set #1, Line 193:</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_rwlock_wrlock(&systemvarslock);<br> ast_rwlock_wrlock(&specialchanvarslock);<br> ast_rwlock_wrlock(&modvarslock);<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I don't think the lists are being modified by this function correct? If so these can be read locks.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17792/comment/8a11f6b5_a1da97d9">Patch Set #1, Line 259:</a> <code style="font-family:monospace,monospace"> ast_rwlock_wrlock(lock);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This can be a read lock.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/17792">change 17792</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/+/17792"/><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-Change-Id: Iad3b8d9833c7d9debe04aca59260d7316c3ad28c </div>
<div style="display:none"> Gerrit-Change-Number: 17792 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: N A <mail@interlinked.x10host.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: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Attention: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 21 Jan 2022 17:27:26 +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: Sean Bright <sean@seanbright.com> </div>
<div style="display:none"> Comment-In-Reply-To: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Comment-In-Reply-To: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>