[Asterisk-code-review] pbx_variables: Add variable registration and validation (asterisk[master])
N A
asteriskteam at digium.com
Thu Jan 13 15:25:30 CST 2022
Attention is currently required from: Kevin Harwell.
N A has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/17792 )
Change subject: pbx_variables: Add variable registration and validation
......................................................................
Patch Set 1:
(5 comments)
File main/pbx_variables.c:
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/b9d0d20f_1d466f3d
PS1, Line 112: AST_RWLOCK_DEFINE_STATIC(systemvarslock);
: AST_RWLOCK_DEFINE_STATIC(specialchanvarslock);
: AST_RWLOCK_DEFINE_STATIC(modvarslock);
: AST_RWLOCK_DEFINE_STATIC(modeditvarslock);
: static struct varshead systemvars = AST_LIST_HEAD_NOLOCK_INIT_VALUE;
: static struct varshead specialchanvars = AST_LIST_HEAD_NOLOCK_INIT_VALUE;
: static struct varshead modvars = AST_LIST_HEAD_NOLOCK_INIT_VALUE;
: static struct varshead modeditvars = AST_LIST_HEAD_NOLOCK_INIT_VALUE;
> Any reason to not just use AST_RWLIST_HEAD_STATIC for each of these vs. […]
No, I just tried to replicate what was going on with globalslock - not really super aware of how some of this works ;)
Could globalslock also use head static or there something different about that?
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/8430f997_6334c19f
PS1, Line 116: static struct varshead systemvars = AST_LIST_HEAD_NOLOCK_INIT_VALUE;
: static struct varshead specialchanvars = AST_LIST_HEAD_NOLOCK_INIT_VALUE;
: static struct varshead modvars = AST_LIST_HEAD_NOLOCK_INIT_VALUE;
: static struct varshead modeditvars = AST_LIST_HEAD_NOLOCK_INIT_VALUE;
> Thinking out loud here, but it would simplify things to just have single container (list or vector) […]
I had thought about - the varshead struct was just accomodating enough that borrowing that seemed to work okay.
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?
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/134ebdd2_c57d8e91
PS1, Line 128: if (!strcmp(ast_var_name(newvariable), nametail)) {
> User defined variables are case-insensitive, so this should be a case compare for those.
Hmm, they're case sensitive in 12+ according to https://wiki.asterisk.org/wiki/display/AST/Case+Sensitivity
Are you referring to something else here?
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/40aa61e1_ddd15a21
PS1, Line 155: int ast_varname_reserved(const char *name)
: {
: char *tmp = (char*) name;
: /* Asterisk broadly reserves all uppercase + underscore var names for use by Asterisk */
: while (*tmp) {
: if (tmp[0] != '_' && !isupper(tmp[0])) {
: return 0;
: }
: tmp++;
: }
: return 1;
: }
> Asterisk currently does not consider upper case variables with a leading underscore as reserved, and […]
Sorry, I meant things like PLAYBACK_STATUS are reserved, not thing like _MYVAR.
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/246b49dc_acc0401e
PS1, Line 184: int ast_bad_variable_assignment(const char *name)
: {
: int res = 0;
:
: if (ast_variable_editable(name)) {
: return 0; /* if not a special reserved format (all uppercase/underscore), okay */
: }
:
: /* variable name is reserved, but is it actually in use? */
: ast_rwlock_wrlock(&systemvarslock);
: ast_rwlock_wrlock(&specialchanvarslock);
: ast_rwlock_wrlock(&modvarslock);
: res |= ast_variable_register_helper(&systemvars, 1, name, NULL);
: res |= ast_variable_register_helper(&specialchanvars, 1, name, NULL);
: res |= ast_variable_register_helper(&modvars, 1, name, NULL);
: ast_rwlock_unlock(&systemvarslock);
: ast_rwlock_unlock(&specialchanvarslock);
: ast_rwlock_unlock(&modvarslock);
:
: if (res) {
: ast_log(LOG_WARNING, "Read-only variable name '%s' is reserved by Asterisk and cannot be set\n", name);
: return -1; /* user cannot set this variable, reject */
: }
:
: ast_log(LOG_WARNING, "Variable name '%s' is reserved by Asterisk (all uppercase) and may pose a conflict\n", name);
: return 0; /* warn the user, but don't outright reject setting the variable */
: }
> Can this whole function just collapse into an "is_readonly" check?
is_readonly is only a small part of the check... part of this is also seeing if the variable is registered or not.
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.
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/17792
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Iad3b8d9833c7d9debe04aca59260d7316c3ad28c
Gerrit-Change-Number: 17792
Gerrit-PatchSet: 1
Gerrit-Owner: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Attention: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Thu, 13 Jan 2022 21:25:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220113/7e58368d/attachment-0001.html>
More information about the asterisk-code-review
mailing list