<p> Attention is currently required from: Kevin Harwell. </p>
<p><a href="https://gerrit.asterisk.org/c/asterisk/+/17792">View Change</a></p><p>5 comments:</p><ul style="list-style: none; padding: 0;"><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/b9d0d20f_1d466f3d">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;">Any reason to not just use AST_RWLIST_HEAD_STATIC for each of these vs. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">No, I just tried to replicate what was going on with globalslock - not really super aware of how some of this works ;)<br>Could globalslock also use head static or there something different about that?</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/8430f997_6334c19f">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;">Thinking out loud here, but it would simplify things to just have single container (list or vector) […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I had thought about - the varshead struct was just accomodating enough that borrowing that seemed to work okay.<br>My one thought is right now the CLI displays the variables sorted by type, and for traversing only the read/only var, we only need to traverse a single list instead of the whole list (which will probably become quite large). So would doing it with one list be less efficient, since it would need to be sorted and traversed for everything?</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/134ebdd2_c57d8e91">Patch Set #1, Line 128:</a> <code style="font-family:monospace,monospace"> if (!strcmp(ast_var_name(newvariable), nametail)) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">User defined variables are case-insensitive, so this should be a case compare for those.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Hmm, they're case sensitive in 12+ according to https://wiki.asterisk.org/wiki/display/AST/Case+Sensitivity<br>Are you referring to something else here?</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/40aa61e1_ddd15a21">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;">Asterisk currently does not consider upper case variables with a leading underscore as reserved, and […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Sorry, I meant things like PLAYBACK_STATUS are reserved, not thing like _MYVAR.</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/246b49dc_acc0401e">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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Can this whole function just collapse into an "is_readonly" check?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">is_readonly is only a small part of the check... part of this is also seeing if the variable is registered or not.<br>This is also a helper function, really, in that it directly logs things before returning. The other APIs are true "getter" functions that could be used anywhere whereas this is a specific helper function for varset check operations. That way, we can log here in one place as opposed to each place where user vars might get set.</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: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 13 Jan 2022 21:25:30 +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: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>