[Asterisk-code-review] pbx_variables: Add variable registration and validation (asterisk[master])
Kevin Harwell
asteriskteam at digium.com
Tue Jan 11 13:14:14 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: Code-Review-1
(14 comments)
File include/asterisk/chanvars.h:
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/0249e7e9_c835bf57
PS1, Line 28: /*!
: * \brief special variable types
: * \note These only apply to special Asterisk variables (i.e. all caps)
: */
: enum ast_variable_type {
: AST_VAR_SYSTEM, /*!< Special variables that pertain to the entire Asterisk system (R/O) */
: AST_VAR_CHANNEL_BUILTIN, /*!< Special variables that pertain to a specific channel (R/O) */
: AST_VAR_MODULE, /*!< Special variables used by individual modules (R/O) */
: AST_VAR_MODULE_WRITE, /*!< Special variables used by individual modules, that a user may write */
: };
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.
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/4c00648f_5cb237e7
PS1, Line 53: int ast_varname_reserved(const char *name);
Conventionally this should be named *_is_reserved.
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/ca56784d_00428616
PS1, Line 63: int ast_variable_editable(const char *name);
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.
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/23fe5327_a566f749
PS1, Line 77: int ast_bad_variable_assignment(const char *name);
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.
File main/pbx_variables.c:
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/32267460_9cfb5e62
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. maintaining the locks separately?
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/44bf6bbe_fead0e19
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) that holds all registered variables. The container holds a new variable type object, for example:
struct {
enum ast_variable_type type; /* Might not be needed */
unsigned int is_readonly; /* Or could be a generic 'permissions' field */
char *description;
char *location; /* what registered it - module or core system name */
ast_var_t variable;
};
Then you can search, filter, etc... on that. I'll let you extrapolate from there.
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/1e7748f9_ff9b7acc
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.
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/162db55d_ea4d103b
PS1, Line 134: if (!add && value) { /* delete an existing var */
: ast_debug(1, "Deregistering special variable '%s'\n", name);
: AST_LIST_REMOVE_CURRENT(entries);
: ast_var_delete(newvariable);
: break;
: }
I think de-registering should be in separate function.
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/104b12ac_a5cc4c7b
PS1, Line 144: if (res && value) {
: ast_log(LOG_WARNING, "Failed to catch duplicate registration of '%s'\n", name); /* should never happen */
: return -1;
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.
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/b05ab2af_81f1a290
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 it's my opinion it should not. At least at this time I can think of a good reason it should.
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/1fcbaff0_d8223974
PS1, Line 177: ast_rwlock_wrlock(&modeditvarslock);
This can be a read lock.
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/4ac6fb86_c2347dfb
PS1, Line 193: ast_rwlock_wrlock(&systemvarslock);
: ast_rwlock_wrlock(&specialchanvarslock);
: ast_rwlock_wrlock(&modvarslock);
I don't think the lists are being modified by this function correct? If so these can be read locks.
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/affe390a_acff98e3
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?
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/ff86c2be_d2ce605b
PS1, Line 259: ast_rwlock_wrlock(lock);
This can be a read lock.
--
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: Tue, 11 Jan 2022 19:14:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220111/44236700/attachment-0001.html>
More information about the asterisk-code-review
mailing list