[Asterisk-code-review] func_evalexten: Extension evaluation function (asterisk[master])
N A
asteriskteam at digium.com
Fri Aug 6 14:40:29 CDT 2021
Attention is currently required from: Joshua Colp, Sarah Autumn, George Joseph.
N A has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/16075 )
Change subject: func_evalexten: Extension evaluation function
......................................................................
Patch Set 10:
(17 comments)
Patchset:
PS1:
> I think this would be a helpful feature.
Ack
Patchset:
PS4:
> I personally still wouldn't like to see it in the tree because I disagree with configuring it from t […]
Done
Patchset:
PS9:
> If I'm understanding it right, this seems to be oriented around fetching a value from the DB and par […]
Done
File funcs/func_lookup.c:
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/ea74cdf7_6466de6c
PS9, Line 39: LOOKUP
> Rename to EVAL_EXTEN
Done
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/b31eb724_b04a508c
PS9, Line 41: Returns the variable-substituted value of an extension in a dialplan context.
> Evaluates the contents of a dialplan extension and returns it as a string.
Done
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/9e97ced9_baaef2ba
PS9, Line 43: <syntax>
: <parameter name="extension" required="true" argsep=",">
: <parameter name="extension" required="true" argsep="@">
: <argument name="extension" required="true" />
: <argument name="context" />
: </parameter>
: <argument name="priority" />
: </parameter>
: </syntax>
> Change to normal [[context,]extensions,]priority syntax.
Done
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/85206adb_0d5b79ee
PS9, Line 52: <description>
> Part of the confusion around this function were the dialplan entries for the data. Rather than […]
Done
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/f5a2da96_ab63f949
PS9, Line 53: <para>The LOOKUP function returns the parsed value of an extension
: in the dialplan. Lookup extensions are no different from ordinary
: extensions, but you may choose to use any arbitrary value or function,
: similar to usage of the HINT function. Any variables in the lookup value
: will be resolved in the context of the lookup context. For example,
: <literal>${EXTEN}</literal> would refer to the LOOKUP extension, not
: the extension in the context invoking the function.</para>
> The EVAL_EXTEN function looks up a dialplan entry by context,extension,priority, evaluates the conte […]
Done
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/b77d0fa6_537857f3
PS9, Line 62: <ref type="function">HINT</ref>
> Referencing HINT is confusing. Reference EVAL instead.
Done
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/3ef37d6f_a6eab843
PS9, Line 72: e = pbx_find_extension(c, NULL, &q, context, exten, priority, NULL, "", E_MATCH);
> You need to check if the priority contains non-numeric characters and if it does, you need to pass t […]
Done
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/694f8976_495db688
PS9, Line 82: ast_get_extension_app(e);
> This won't work if the value contains any parentheses. […]
Done
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/48248d2f_c8482822
PS9, Line 67: tatic struct ast_exten *ast_lookup_extension(struct ast_channel *c, const char *context, const char *exten, int priority)
: {
: struct ast_exten *e;
: struct pbx_find_info q = { .stacklen = 0 }; /* the rest is set in pbx_find_context */
: ast_rdlock_contexts();
: e = pbx_find_extension(c, NULL, &q, context, exten, priority, NULL, "", E_MATCH);
: ast_unlock_contexts();
: return e;
: }
:
: static int ast_get_lookup(char *lookup, int lookupsize, char *name, int namesize, struct ast_channel *c, const char *context, const char *exten, int priority)
: {
: struct ast_exten *e = ast_lookup_extension(c, context, exten, priority);
: if (e) {
: if (name) {
: const char *tmp = ast_get_extension_app(e);
: if (tmp)
: ast_copy_string(name, tmp, namesize);
: }
: return 0;
: }
: return -1;
: }
:
> Collapse these two functions. […]
Done
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/752ceba2_e0ffb4a4
PS9, Line 99: AST_DECLARE_APP_ARGS(args,
> Refactor to use the standard [[context,]extensions,]priority syntax.
Done
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/81daceab_621d707c
PS9, Line 116: context = exten = args.exten;
: strsep(&context, "@");
: if (ast_strlen_zero(context))
: context = "default";
> Default context should be "this" context and default exten should be "this" extension, like it is in […]
Done
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/154645f2_82702c3c
PS9, Line 121: priority = atoi(args.priority);
> priority can be a label instead of a number.
Done
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/e21ae604_87230e6d
PS9, Line 127: /* Substitute variables now, using the location of the lookup */
: /* strdupa required or we'll just overwrite what we read when we set these */
: realcontext = ast_strdupa(ast_channel_context(chan));
: realexten = ast_strdupa(ast_channel_exten(chan));
: realpriority = ast_channel_priority(chan);
: ast_channel_lock(chan);
: ast_channel_context_set(chan, context);
: ast_channel_exten_set(chan, exten);
: ast_channel_priority_set(chan, priority);
: ast_channel_unlock(chan);
:
> Thinking more about this... Just leave it the way you have it. […]
Would this actually work? If an EVAL_EXTEN calls another EVAL_EXTEN, then the channel is already locked so the second one will fail. What is the downside if the channel is not locked?
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/e073792a_87ff5dcd
PS9, Line 127: /* Substitute variables now, using the location of the lookup */
: /* strdupa required or we'll just overwrite what we read when we set these */
: realcontext = ast_strdupa(ast_channel_context(chan));
: realexten = ast_strdupa(ast_channel_exten(chan));
: realpriority = ast_channel_priority(chan);
: ast_channel_lock(chan);
: ast_channel_context_set(chan, context);
: ast_channel_exten_set(chan, exten);
: ast_channel_priority_set(chan, priority);
: ast_channel_unlock(chan);
:
> Thinking more about this... Just leave it the way you have it. […]
Done
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/16075
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Iad81019689674c9f4ac77d235f5d7234adbb1432
Gerrit-Change-Number: 16075
Gerrit-PatchSet: 10
Gerrit-Owner: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Sarah Autumn <sarah at endlesstemple.org>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Sarah Autumn <sarah at endlesstemple.org>
Gerrit-Attention: George Joseph <gjoseph at digium.com>
Gerrit-Comment-Date: Fri, 06 Aug 2021 19:40:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Joshua Colp <jcolp at sangoma.com>
Comment-In-Reply-To: N A <mail at interlinked.x10host.com>
Comment-In-Reply-To: Sarah Autumn <sarah at endlesstemple.org>
Comment-In-Reply-To: George Joseph <gjoseph at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210806/aedd47c5/attachment.html>
More information about the asterisk-code-review
mailing list