[Asterisk-code-review] app_waitforcond: New application (asterisk[16])

[Kevin Harwell]

Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/15928 )

Change subject: app_waitforcond: New application
......................................................................


Patch Set 1: Code-Review-1

(6 comments)

Ready through the code I'm still not sure what an example might look like and why the substitution is being done the way it is. Could you provide an example dialplan of what a call would look like?

https://gerrit.asterisk.org/c/asterisk/+/15928/1/apps/app_waitforcond.c 
File apps/app_waitforcond.c:

https://gerrit.asterisk.org/c/asterisk/+/15928/1/apps/app_waitforcond.c@55 
PS1, Line 55: 			<parameter name="timeout" />
            : 			<parameter name="interval" />
Add the unit type for these, and if seconds mention if fractions of a second are supported.


https://gerrit.asterisk.org/c/asterisk/+/15928/1/apps/app_waitforcond.c@82 
PS1, Line 82: static int waitforcond_exec(struct ast_channel *chan, const char *data)
Some of the failures paths return 0, while others return -1. Should all return -1?

Also some failure paths set WAITFORCONDITIONSTATUS to FAILURE while other failure paths do not. Should they all set the status?


https://gerrit.asterisk.org/c/asterisk/+/15928/1/apps/app_waitforcond.c@94 
PS1, Line 94: 	
Remove whitespace


https://gerrit.asterisk.org/c/asterisk/+/15928/1/apps/app_waitforcond.c@107 
PS1, Line 107: 	if (ast_strlen_zero(tmp) || tmp[0] == '\0') {
The second check is already found in the ast_strlen_zero function so can be removed.


https://gerrit.asterisk.org/c/asterisk/+/15928/1/apps/app_waitforcond.c@113 
PS1, Line 113: 	if (!strchr(tmp, '[') && !strchr(tmp, ']')) {
As you currently have it a string like "ab]cd[ef" would pass your check.

If you want to make this more robust you might want to start the second search for the closing bracket start from the found position of the first search. For example something like:

if (!(open_bracket = strchr(tmp, '[')) && !strchr(open_bracket, ']'))


https://gerrit.asterisk.org/c/asterisk/+/15928/1/apps/app_waitforcond.c@203 
PS1, Line 203: 		cond = ast_strdupa(condition);
Do not call strdupa within a loop as it could blow the stack.

You either have to create/free memory on the heap, or you a variable array.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/15928
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: I08adf2824b8bc63405778cf355963b5005612f41
Gerrit-Change-Number: 15928
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-CC: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Tue, 25 May 2021 22:31:07 +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/20210525/e2a710e6/attachment.html>