[Asterisk-code-review] pbx_variables: Add variable registration and validation (asterisk[master])
N A
asteriskteam at digium.com
Fri Jan 21 11:27:26 CST 2022
Attention is currently required from: Sean Bright, Joshua Colp, 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 3:
(12 comments)
File include/asterisk/chanvars.h:
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/064cc5a9_16934b05
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 th […]
Done
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/002e88af_9114186c
PS1, Line 53: int ast_varname_reserved(const char *name);
> Conventionally this should be named *_is_reserved.
Done
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/5ac4e583_162b4133
PS1, Line 63: int ast_variable_editable(const char *name);
> Same as above, conventionally this should be named *_is_editable. […]
Done
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/d8a6e180_d7f5a824
PS1, Line 77: int ast_bad_variable_assignment(const char *name);
> This should start with "ast_variable_". […]
Done
File main/pbx_variables.c:
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/2979aa9a_458638ea
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;
> At a glance I do not see anything different about it, or see why it too could not be refactored to u […]
Done
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/0dc76299_14794a7c
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'll look into the vector approach. […]
Ack
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/2e30ebc1_9eac4d70
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.
Done
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/3dd9f263_17dec8ef
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. […]
Done
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/498a8a83_99e3660f
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;
: }
> > Au contraire, that *has* been the longstanding precedent […]
Ack
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/7ed7ee77_7f64546a
PS1, Line 177: ast_rwlock_wrlock(&modeditvarslock);
> This can be a read lock.
Done
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/51736f53_fd3dd307
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.
Done
https://gerrit.asterisk.org/c/asterisk/+/17792/comment/8a11f6b5_a1da97d9
PS1, Line 259: ast_rwlock_wrlock(lock);
> This can be a read lock.
Done
--
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: 3
Gerrit-Owner: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Sean Bright <sean at seanbright.com>
Gerrit-CC: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Fri, 21 Jan 2022 17:27:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sean Bright <sean at seanbright.com>
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/20220121/c5bae2f5/attachment-0001.html>
More information about the asterisk-code-review
mailing list