[Asterisk-code-review] func_lookup: Fork of HINT function (asterisk[master])
George Joseph
asteriskteam at digium.com
Thu Aug 5 10:42:29 CDT 2021
Attention is currently required from: N A.
George Joseph has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/16075 )
Change subject: func_lookup: Fork of HINT function
......................................................................
Patch Set 9:
(14 comments)
Patchset:
PS9:
Addressing these items will be necessary to get this review merged...
File funcs/func_lookup.c:
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/231e6ee6_bf2d3ed0
PS9, Line 39: LOOKUP
Rename to EVAL_EXTEN
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/98614f86_50224a3c
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.
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/4af621df_013e218e
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.
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/590e2f51_a6de8749
PS9, Line 52: <description>
Part of the confusion around this function were the dialplan entries for the data. Rather than
exten = _X.,1,<data>
wrap the data in a Return like so...
exten = _X.,1,Return(<data>)
This makes it much clearer what the intent is and it allows the use of Gosub and a possible future CALL_SUB function. In the case of EVAL_EXTEN, the Return isn't actually executed. It also fixes another issue in that ast_get_extension_app() will only return up to the first '(' and ignore the rest. If you use "Return(<data>)" and change the call of ast_get_extension_app() to ast_get_extension_app_data(), you'll get all of <data> even if it contains more parentheses.
Finally provide examples!
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/78407f48_318a36c2
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 contents of a Return statement to resolve any variable or function references, and returns the result as a string. you can use this function to create simple user-defined lookup tables or user-defined functions.
Example:
[call-types]
exten = _1nnn,1,Return(internal)
exten = _NXXNXXXXXX,1,Return(external)
[udf]
exten = calleridlen,1,Return(${LEN(${CALLERID(num)})})
[default]
exten = _X.,1,Verbose(Call type ${EVAL_EXTEN(call-types,${EXTEN},1)})
same = n,Verbose(Call type ${EVAL_EXTEN(udf,calleridlen,1)})
Limitations:
Because the contents of the extension aren't executed like a Gosub, you can't pass arguments in the EVAL_EXTEN function.
----Of course, format with appropriate XML.----
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/e2f51755_e59bf823
PS9, Line 62: <ref type="function">HINT</ref>
Referencing HINT is confusing. Reference EVAL instead.
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/3160a994_71c09250
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 the priority in the pbx_find_extension "label" parameter and set the match type to E_FINDLABEL. See func_dialplan.
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/d9281b41_f7b4661f
PS9, Line 82: ast_get_extension_app(e);
This won't work if the value contains any parentheses.
exten = 555,1,abc(def)ghi
will only return "abc".
Use ast_get_extension_app_data() as noted above.
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/f0eed0ab_37e9de8c
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.
Wrap long lines.
Remove the ast_.
Remove unused function parameters.
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/d62aeb24_5100f517
PS9, Line 99: AST_DECLARE_APP_ARGS(args,
Refactor to use the standard [[context,]extensions,]priority syntax.
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/a20fb459_d4468d2e
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 other calls.
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/04aa1a6b_17e41ede
PS9, Line 121: priority = atoi(args.priority);
priority can be a label instead of a number.
https://gerrit.asterisk.org/c/asterisk/+/16075/comment/6ecd84bd_8cefa5db
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);
:
I don't understand this. I thought the objective was to keep the original context, extension and priority, not the lookup target ones?
In any case, any time you call one of the set functions on a channel, the channel snapshot is invalidated which could result in extra AMI and ARI events being generated and potentially cause ARI event listeners to fire.
That's not good. If you really want the lookup target's data, you'll have to find another way that doesn't modify the channel.
--
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: 9
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: N A <mail at interlinked.x10host.com>
Gerrit-Comment-Date: Thu, 05 Aug 2021 15:42:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210805/b15898b0/attachment.html>
More information about the asterisk-code-review
mailing list