[Asterisk-code-review] pbx_variables: Add variable registration and validation (asterisk[master])
Kevin Harwell
asteriskteam at digium.com
Thu Jan 13 17:49:11 CST 2022
Attention is currently required from: N A.
Kevin Harwell 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/418b58d0_cc0c00b3
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;
> I had thought about - the varshead struct was just accomodating enough that borrowing that seemed to […]
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 :-)
If concerned about performance though, and if the container doesn't change much after initial load you might consider switching things to a vector.
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/a460fce7_e0f04ff8
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;
> No, I just tried to replicate what was going on with globalslock - not really super aware of how som […]
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 .
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/a58917bd_0fd5d473
PS1, Line 128: if (!strcmp(ast_var_name(newvariable), nametail)) {
> Hmm, they're case sensitive in 12+ according to https://wiki.asterisk. […]
Oh man totally missed that. Just glad I hadn't actually responded on the original discussion :-D
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/b94dc19f_2f97f015
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;
: }
> Sorry, I meant things like PLAYBACK_STATUS are reserved, not thing like _MYVAR.
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.
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.
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.
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/3d38737d_d78c027e
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 */
: }
> is_readonly is only a small part of the check... […]
Ack, perhaps rename then to make it more clear. Something like ast_variable_validate.
--
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: N A <mail at interlinked.x10host.com>
Gerrit-Comment-Date: Thu, 13 Jan 2022 23:49:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: N A <mail at interlinked.x10host.com>
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/761a8a37/attachment.html>
More information about the asterisk-code-review
mailing list