[Asterisk-code-review] app originate: Add option to execute gosub prior to dial (asterisk[master])

[David Kerr]

David Kerr has posted comments on this change. ( https://gerrit.asterisk.org/4484 )

Change subject: app_originate: Add option to execute gosub prior to dial
......................................................................


Patch Set 4:

(10 comments)

Responded to all code review comments.  See above.
David

https://gerrit.asterisk.org/#/c/4484/1/CHANGES
File CHANGES:

Line 64: 
> Extra redness
Done


https://gerrit.asterisk.org/#/c/4484/1/apps/app_originate.c
File apps/app_originate.c:

Line 217: 		ast_debug(1, "Predial caller '%s' \n",opt_args[OPT_ARG_PREDIAL_CALLER]);
> Can this debug message be extended to be a bit more useful? Extension/diale
I'm not sure what more info would be useful.  If you have debug & verbose turned on you will see all the parameters as the Originate() app is called from the dialplan.  Including for that matter the info I included in this debug message -- which I put here purely to assure myself that my new code was doing what I expected as I developed it.  The right answer might be to remove the debug messages completely?


Line 220: 
> Redness
Done


Line 227: 
> Redness
Done


https://gerrit.asterisk.org/#/c/4484/1/include/asterisk/pbx.h
File include/asterisk/pbx.h:

Line 1179: int __ast_pbx_outgoing_exten(const char *type, struct ast_format_cap *cap, const char *addr,
> The prefix __ is generally used to indicate an internal API function at tim
I modeled this after ast_request_and_dial() [in channel.h / channel.c] where exactly the same thing is done (adding a parameter) and in this case they use the __prefix and (in Asterisk 11) the __version is called from pbx.c so it was not strictly internal to channel.c.  Okay, that said, if the __prefix is bad practice in Asterisk 13+ then certainly easy to change to using suffix at the end.  Please advise which you prefer I use and I will either leave as is or change per your instruction.  Thanks.


https://gerrit.asterisk.org/#/c/4484/1/main/pbx.c
File main/pbx.c:

Line 7641: 
> Redness
Done


Line 7666: 
> Redness
Done


Line 7667: 	if (!ast_strlen_zero(predial_callee)) {
> Should this use ast_strlen_zero instead?
So, I tried to convince myself that simply testing for non-NULL was okay here given that the only place that predial_callee is set to anything other than NULL is known to us (and there is a ast_strlen_zero test there).  But that assumes that nothing changes in the future (some other code path ends up using this capability) so for safeness I think you are right... change this to !ast_strlen_zero(predial_callee)


https://gerrit.asterisk.org/#/c/4484/2/main/pbx.c
File main/pbx.c:

PS2, Line 7641: 
Humm, did not get removed


https://gerrit.asterisk.org/#/c/4484/3/main/pbx.c
File main/pbx.c:

PS3, Line 7697: !ast_strlen_ze
need to change this to ast_strlen_zero as well


-- 
To view, visit https://gerrit.asterisk.org/4484
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I36abc39b58567ffcab4a636ea196ef48be234c57
Gerrit-PatchSet: 4
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: David Kerr <david at kerr.net>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: David Kerr <david at kerr.net>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-HasComments: Yes