[Asterisk-code-review] func_framedrop: New function (asterisk[master])

[N A]

Attention is currently required from: Joshua Colp, George Joseph.
N A has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/16071 )

Change subject: func_framedrop: New function
......................................................................


Patch Set 2:

(8 comments)

File funcs/func_framedrop.c:

https://gerrit.asterisk.org/c/asterisk/+/16071/comment/bb592f3b_59762def 
PS2, Line 1: /*
> The source file name should probably be func_frame_drop. […]
Ack


https://gerrit.asterisk.org/c/asterisk/+/16071/comment/d6c3163f_dd73e322 
PS2, Line 42: Intercepts specified frames on a channel instead of passing them on.
> "Drops specific frame types in the <literal>TX</literal> or <literal>RX</literal> direction on a cha […]
Ack


https://gerrit.asterisk.org/c/asterisk/+/16071/comment/c0fe6023_5e18e8ba 
PS2, Line 46: A filter can be applied to the trace to limit what frames are suppressed. This
            : 				filter can be a <literal>TX</literal> or <literal>RX</literal> list of frame types
            : 				and control frame types.
> How about... "List of frame types to be dropped for the specified direction. […]
Ack


https://gerrit.asterisk.org/c/asterisk/+/16071/comment/5f66e53b_4f7f9091 
PS2, Line 67: 					<enum name = "HANGUP" />
> I am not comfortable with this list of CONTROL frames. […]
Are there any others you had in mind besides hangup?
My main use cases are dropping DTMF, flash, and answer frames, but I wanted to leave other permissible options open. I'm not sure what else here would be considered a "no no".


https://gerrit.asterisk.org/c/asterisk/+/16071/comment/2a81cb97_8d379b88 
PS2, Line 124: static struct {
> The lists are a little fragile since someone could add a new frametype and forget to update these li […]
Ack


https://gerrit.asterisk.org/c/asterisk/+/16071/comment/d7cfc9c8_060ce31c 
PS2, Line 195: int list_type; /* 0 = TX, 1 = RX */
> Can you make this an enum?
Are you referring to list_type or values here? I don't see a red line in the excerpt.


https://gerrit.asterisk.org/c/asterisk/+/16071/comment/8c378c4a_5c4ad6d0 
PS2, Line 278: 	for (i = 0; i < ARRAY_LEN(frametype2str); i++) {
             : 		if (strcasestr(value, frametype2str[i].str)) {
             : 			framedata->values[i] = 1;
             : 		}
             : 	}
             : 
             : 	for (i = 0; i < ARRAY_LEN(controlframetype2str); i++) {
             : 		if (strcasestr(value, controlframetype2str[i].str)) {
             : 			framedata->controlvalues[i] = 1;
             : 		}
             : 	}
> I think these are going to be an issue because if a user specified "TEXT_DATA", both "TEXT" and "TEX […]
Whoops, didn't notice that before. What about strcasecmp instead of strcasestr?


https://gerrit.asterisk.org/c/asterisk/+/16071/comment/b9e8be0e_8e972abf 
PS2, Line 294: 		if ((datastore = ast_channel_datastore_find(chan, &frame_drop_datastore, NULL))) {
             : 			id = datastore->data;
             : 			ast_framehook_detach(chan, *id);
             : 			ast_channel_datastore_remove(chan, datastore);
             : 			ast_datastore_free(datastore);
             : 		}
             : 
> It'd probably be good to add a note to the XML documentation that subsequent calls to FRAME_DROP wil […]
Added.
Also, something I noticed from testing is that the directions seem to work backwards from other functions. For instance, to prevent DTMF from registering in Asterisk from an endpoint, Set(FRAME_DROP(TX)=DTMF_END,DTMF_BEGIN) is the way that does it. This seems backwards to me at least - wondering if the TX/RX should be flipped around in the code so that it works the other way or if there should be a note about this.



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I8147c9d55d74e2e48861edba6b22f930920541ec
Gerrit-Change-Number: 16071
Gerrit-PatchSet: 2
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-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: George Joseph <gjoseph at digium.com>
Gerrit-Comment-Date: Mon, 21 Jun 2021 13:18:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Joshua Colp <jcolp at sangoma.com>
Comment-In-Reply-To: George Joseph <gjoseph at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210621/8a75de3a/attachment-0001.html>