<p> Attention is currently required from: N A. </p>
<p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/17792">View Change</a></p><p>14 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/0249e7e9_c835bf57">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 style="white-space: pre-wrap; word-wrap: break-word;">See a later comment about creating a new variable structure, but if you do do something like that then the type of variable may no longer be needed, except maybe for logging and/or CLI purposes.</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/4c00648f_5cb237e7">Patch Set #1, Line 53:</a> <code style="font-family:monospace,monospace">int ast_varname_reserved(const char *name);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Conventionally this should be named *_is_reserved.</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/ca56784d_00428616">Patch Set #1, Line 63:</a> <code style="font-family:monospace,monospace">int ast_variable_editable(const char *name);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same as above, conventionally this should be named *_is_editable. However, just the way my brain thinks, but I'd prefer the negation is_readonly.</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/23fe5327_a566f749">Patch Set #1, Line 77:</a> <code style="font-family:monospace,monospace">int ast_bad_variable_assignment(const char *name);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should start with "ast_variable_". Similar to the others though I think this should be something like ast_variable_is_bad, or ast_variable_can_assign.</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/32267460_9cfb5e62">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 style="white-space: pre-wrap; word-wrap: break-word;">Any reason to not just use AST_RWLIST_HEAD_STATIC for each of these vs. maintaining the locks separately?</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/44bf6bbe_fead0e19">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 style="white-space: pre-wrap; word-wrap: break-word;">Thinking out loud here, but it would simplify things to just have single container (list or vector) that holds all registered variables. The container holds a new variable type object, for example:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">struct {<br> enum ast_variable_type type; /* Might not be needed */<br> unsigned int is_readonly; /* Or could be a generic 'permissions' field */<br> char *description;<br> char *location; /* what registered it - module or core system name */<br> ast_var_t variable;<br>};</pre><p style="white-space: pre-wrap; word-wrap: break-word;">Then you can search, filter, etc... on that. I'll let you extrapolate from there.</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/1e7748f9_ff9b7acc">Patch Set #1, Line 128:</a> <code style="font-family:monospace,monospace"> if (!strcmp(ast_var_name(newvariable), nametail)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">User defined variables are case-insensitive, so this should be a case compare for those.</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/162db55d_ea4d103b">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 style="white-space: pre-wrap; word-wrap: break-word;">I think de-registering should be in separate function.</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/104b12ac_a5cc4c7b">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 style="white-space: pre-wrap; word-wrap: break-word;">From above it appears the only way res != 0 is if !value. Based on that this will never be true so this condition can be removed.</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/b05ab2af_81f1a290">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 style="white-space: pre-wrap; word-wrap: break-word;">Asterisk currently does not consider upper case variables with a leading underscore as reserved, and it's my opinion it should not. At least at this time I can think of a good reason it should.</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/1fcbaff0_d8223974">Patch Set #1, Line 177:</a> <code style="font-family:monospace,monospace"> ast_rwlock_wrlock(&modeditvarslock);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This can be a read lock.</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/4ac6fb86_c2347dfb">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 style="white-space: pre-wrap; word-wrap: break-word;">I don't think the lists are being modified by this function correct? If so these can be read locks.</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/affe390a_acff98e3">Patch Set #1, Line 184:</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_bad_variable_assignment(const char *name)<br>{<br> int res = 0;<br><br> if (ast_variable_editable(name)) {<br> return 0; /* if not a special reserved format (all uppercase/underscore), okay */<br> }<br><br> /* variable name is reserved, but is it actually in use? */<br> ast_rwlock_wrlock(&systemvarslock);<br> ast_rwlock_wrlock(&specialchanvarslock);<br> ast_rwlock_wrlock(&modvarslock);<br> res |= ast_variable_register_helper(&systemvars, 1, name, NULL);<br> res |= ast_variable_register_helper(&specialchanvars, 1, name, NULL);<br> res |= ast_variable_register_helper(&modvars, 1, name, NULL);<br> ast_rwlock_unlock(&systemvarslock);<br> ast_rwlock_unlock(&specialchanvarslock);<br> ast_rwlock_unlock(&modvarslock);<br><br> if (res) {<br> ast_log(LOG_WARNING, "Read-only variable name '%s' is reserved by Asterisk and cannot be set\n", name);<br> return -1; /* user cannot set this variable, reject */<br> }<br><br> ast_log(LOG_WARNING, "Variable name '%s' is reserved by Asterisk (all uppercase) and may pose a conflict\n", name);<br> return 0; /* warn the user, but don't outright reject setting the variable */<br>}<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Can this whole function just collapse into an "is_readonly" check?</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/ff86c2be_d2ce605b">Patch Set #1, Line 259:</a> <code style="font-family:monospace,monospace"> ast_rwlock_wrlock(lock);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This can be a read lock.</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: 1 </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-Attention: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 11 Jan 2022 19:14:14 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>