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

[George Joseph]

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

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


Patch Set 2: Code-Review-1

(8 comments)

File funcs/func_framedrop.c:

https://gerrit.asterisk.org/c/asterisk/+/16071/comment/a0f52e35_aaa8ab7f 
PS2, Line 1: /*
The source file name should probably be func_frame_drop.c to make it consistent with func_frame_trace.c


https://gerrit.asterisk.org/c/asterisk/+/16071/comment/e2d8fa69_146b8d64 
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 channel.


https://gerrit.asterisk.org/c/asterisk/+/16071/comment/222f77cd_e7decd4c 
PS2, Line 45: ilter list type
How about "direction"?


https://gerrit.asterisk.org/c/asterisk/+/16071/comment/71443768_9c589763 
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.  Direction can be <literal>TX</literal> or <literal>RX</literal>."


https://gerrit.asterisk.org/c/asterisk/+/16071/comment/d893b3f0_f7c47b5a 
PS2, Line 124: static struct {
The lists are a little fragile since someone could add a new frametype and forget to update these lists but I know that's how func_frame_trace does it.   Maybe at some later time you could create a generic name mapping function for everyone to use. :)


https://gerrit.asterisk.org/c/asterisk/+/16071/comment/94f8e533_eb3b8ce7 
PS2, Line 195: int list_type; /* 0 = TX, 1 = RX */
Can you make this an enum?


https://gerrit.asterisk.org/c/asterisk/+/16071/comment/565c2bdc_a775d6e4 
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 "TEXT_DATA" will match.  I think you're better off using a single container to store your string/type cross references, tokenizing "value" and doing a ao2_find.


https://gerrit.asterisk.org/c/asterisk/+/16071/comment/74eb6ece_c6d1dd1a 
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 will replace earlier ones.



-- 
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-Attention: N A <mail at interlinked.x10host.com>
Gerrit-Comment-Date: Mon, 21 Jun 2021 12:57:48 +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/20210621/5a52adfa/attachment-0001.html>