[Asterisk-code-review] funcs/func holdintercept: Actually add the HOLD INTERCEPT fu... (asterisk[master])

[Matt Jordan]

Matt Jordan has posted comments on this change.

Change subject: funcs/func_holdintercept: Actually add the HOLD_INTERCEPT function
......................................................................


Patch Set 2:

(3 comments)

https://gerrit.asterisk.org/#/c/1454/2/funcs/func_holdintercept.c
File funcs/func_holdintercept.c:

Line 87: 		ast_log(AST_LOG_WARNING, "Cannot remove HOLD_INTERCEPT from %s: HOLD_INTERCEPT not currently enabled\n",
       : 		        ast_channel_name(chan));
> This maybe should be just a debug message as the warning could be considere
This is actually in line with warnings we emit in other functions. For example, func_talkdetect does this. Typically when a function is invoke with any kind of invalid input, we also emit warnings. See func_jitterbuffer for other examples of that.


Line 120: 	if (frame_type != AST_CONTROL_HOLD && frame_type != AST_CONTROL_UNHOLD) {
> You should be checking that f->frametype != AST_FRAME_CONTROL before checki
Good catch; thanks.


Line 156: 		ast_log(AST_LOG_WARNING, "HOLD_INTERCEPT already set on '%s'\n",
        : 		        ast_channel_name(chan));
> This likely should be a debug message.  The warning message could be consid
I would tend to disagree, as this is inline with other WARNINGs in other functions.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ida38278f259dd07c334a36f9b7d5475b5db72396
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
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