[Asterisk-code-review] app dial: Add the "Q" option to set the cause on unanswered... (asterisk[13])

[George Joseph]

George Joseph has posted comments on this change.

Change subject: app_dial:  Add the "Q" option to set the cause on unanswered channels
......................................................................


Patch Set 4:

(14 comments)

https://gerrit.asterisk.org/#/c/4033/1/apps/app_dial.c
File apps/app_dial.c:

Line 388: 						You can also specify <literal>0</literal> or <literal>NONE</literal>
> This is the same value you can give to the Hangup application.
True, but Hangup doesn't document them either.


Line 2797: 		pbx_builtin_setvar_helper(chan, "DIALSTATUS", pa.status);
> Use strcasecmp() since ast_str2cause() is not case sensitive.
Done


Line 2800: 		   conversation.  */
> Check if returned -1 and issue a warning about the user's typing/spelling.
Done


Line 3209: 
> I wonder if we should propagate the Q() cause if we get that cause.  This i
Let's chat about this one.


Line 3210: 	if (delprivintro && ast_fileexists(pa.privintro, NULL, NULL) > 0) {
> spaces around == operator
Done


Line 3211: 		ast_filedelete(pa.privintro, NULL);
> Extra space before ) at end of line.
Done


https://gerrit.asterisk.org/#/c/4033/2/apps/app_dial.c
File apps/app_dial.c:

PS2, Line 389: 						to send no cause.  See the <filename>causes.h</filename> file for the
             : 						full list of valid causes and names.
> Asking someone to look in the code (causes.h) isn't a good thing.  However,
Exactly.  It's better than nothing.


Line 538: 			<example title="Dial with call length limit">
> ANSWERED_ELSEWHERE typo
Done


PS2, Line 2802: 		if (!ast_strlen_zero(opt_args[OPT_ARG_HANGUPCAUSE])) {
              : 			cause = ast_
> Have you tested what "NONE"/0 is going to do?  It is likely going to result
I'verified with wireshark that it sets the Reason header code to 0 (not supplied or not available some similar wording).


Line 2808: 					|| cause < 0) {
> Probably should set cause to -1 since they didn't supply a valid substitute
Done


https://gerrit.asterisk.org/#/c/4033/4/apps/app_dial.c
File apps/app_dial.c:

Line 389: 						to send no cause.  See the <filename>causes.h</filename> file for the
> cause.h is not going to give you the strings you can use.  The strings are 
causes.h is close enough in that you only need to remove the AST_CAUSE_ from the front and it's better than sending someone to channel.c


Line 393: 						<para>Only chan_pjsip supports setting the cause to anything other than ANSWERED_ELSEWHERE.</para>
> I don't think this is true at all.  I see in chan_sip, and chan_iax2 among 
It is true and confirmed by testing.  chan_sip will not even send a cause on a CANCEL at all UNLESS this option is set and even then, it will force it to ANSWERED ELSEWHERE.

You got me on chan_iax2.  I didn't test it.


Line 541: 			<example title="Dial alice and bob and send NO_ANSWER to bob instead of ANSWERED_ELSEWHRE when alice answers">
> Typo still here
I swear i fixed this.


Line 2811: 					cause = 0;
> Change to assigning -1.  They have provided a bogus cause so we should defa
I swear I fixed this as well.  Hmmm.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I71742e0919aaa16784c30a2b2e73fbeed7672e47
Gerrit-PatchSet: 4
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Badalian Vyacheslav <v.badalyan at open-bs.ru>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes