<p> Attention is currently required from: N A. </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/418b58d0_cc0c00b3">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 had thought about - the varshead struct was just accomodating enough that borrowing that seemed to […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Unless there are 10's of thousands or more you probably don't have to worry much about performance one way vs the other. I'd pick a way that seems the most readable and maintainable. Alas, arguments could probably be made for either way :-)</p><p style="white-space: pre-wrap; word-wrap: break-word;">If concerned about performance though, and if the container doesn't change much after initial load you might consider switching things to a vector.</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/a460fce7_e0f04ff8">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;">No, I just tried to replicate what was going on with globalslock - not really super aware of how som […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">At a glance I do not see anything different about it, or see why it too could not be refactored to use AST_RWLIST_HEAD_STATIC .</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/a58917bd_0fd5d473">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;">Hmm, they're case sensitive in 12+ according to https://wiki.asterisk. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Oh man totally missed that. Just glad I hadn't actually responded on the original discussion :-D</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/b94dc19f_2f97f015">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;">Sorry, I meant things like PLAYBACK_STATUS are reserved, not thing like _MYVAR.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I misread the code a bit here. But now I see it appears to return 0 (i.e. not reserved) if _any_ character in "name" is not an underscore and is lowercase. So basically all variable uppercase names are considered reserved, while variables containing at least one lowercase are not. Both types can include underscores anywhere in the name.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Whatever the case Asterisk currently doesn't make any such restrictions, and I don't think it should. A list of reserved names might be acceptable, but then the question of scope applies.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Even the latter option would need buy in from the community at large (or at least some discussion on the dev list) before moving forward.</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/3d38737d_d78c027e">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;">is_readonly is only a small part of the check... […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack, perhaps rename then to make it more clear. Something like ast_variable_validate.</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: Thu, 13 Jan 2022 23:49:11 +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: 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>