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

[George Joseph]

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

Change subject: func_frame_drop: New function
......................................................................


Patch Set 4: Code-Review-1

(5 comments)

File funcs/func_frame_drop.c:

https://gerrit.asterisk.org/c/asterisk/+/16071/comment/5d18e310_46edfd36 
PS4, Line 45: filter list type
You still should change this to "direction".


https://gerrit.asterisk.org/c/asterisk/+/16071/comment/5c7ed371_6a2580b6 
PS4, Line 276: strcasecmp
Per my other comment, you can't use strcasecmp because "value" could have multiple frame types in it.


File funcs/func_framedrop.c:

https://gerrit.asterisk.org/c/asterisk/+/16071/comment/ebccb4c6_133b2ddf 
PS2, Line 195: int list_type; /* 0 = TX, 1 = RX */
> Are you referring to list_type or values here? I don't see a red line in the excerpt.
Sorry.  I meant create an enum for the list_type...
enum direction {
    TX = 0,
    RX,
};

struct frame_drop_data {
    enum direction list_type;
...
};


https://gerrit.asterisk.org/c/asterisk/+/16071/comment/5ad1950b_a9a23017 
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;
             : 		}
             : 	}
> Whoops, didn't notice that before. What about strcasecmp instead of strcasestr?

Sure you can do strcasecmp but you'll still have to tokenize the "value" so you're not trying to compare "TEXT" with "TEXT_DATA,VIDEO".  Another option I've used in other places is to allocate a buffer that's long enough to hold "value" plus 2 commas, then place a comma in the first character, copy "value" into the buffer character by character skipping spaces starting at the second character, then place a comma after that so your new value looks like ",TEXT,TEXT_DATA,VIDEO,".  Now change your array string values to have a comma at the start and end and you can do your strcasestr() without tokenizing.  You'll be searching for ",TEXT," in ",TEXT_DATA,VIDEO," and you won't get a false match on ",TEXT_DATA,".


https://gerrit.asterisk.org/c/asterisk/+/16071/comment/6275b95d_27db523b 
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);
             : 		}
             : 
> 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.

I'd make it consistent with the other functions and add the note.



-- 
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: 4
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: N A <mail at interlinked.x10host.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Tue, 22 Jun 2021 12:51:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: N A <mail at interlinked.x10host.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/20210622/52dae3b6/attachment-0001.html>