[Asterisk-code-review] func_scramble: Audio scrambler function (asterisk[master])

[Kevin Harwell]

Attention is currently required from: N A, Richard Mudgett.
Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/16231 )

Change subject: func_scramble: Audio scrambler function
......................................................................


Patch Set 5: Code-Review-1

(3 comments)

File funcs/func_scramble.c:

https://gerrit.asterisk.org/c/asterisk/+/16231/comment/a6294142_50fac772 
PS5, Line 205: 	} else if (!strcasecmp(args.direction, "remove")) {
             : 		return remove_scrambler(chan);
Check this case first prior even to the datastore find above. Otherwise there is a datastore leak here when removing on first call.


https://gerrit.asterisk.org/c/asterisk/+/16231/comment/b34975cb_8fbe69aa 
PS5, Line 208: 		ast_log(LOG_ERROR, "Direction must be either RX, TX or remove\n");
             : 		return -1;
datastore can be leaked here if created, but not added.


https://gerrit.asterisk.org/c/asterisk/+/16231/comment/f6df5bd1_e84b7e50 
PS5, Line 212: 	if (is_new) {
             : 		datastore->data = ni;
             : 		ast_channel_lock(chan);
             : 		ast_channel_datastore_add(chan, datastore);
             : 		ast_channel_unlock(chan);
             : 		ast_audiohook_attach(chan, &ni->audiohook);
             : 	}
I think it'd read easier if you moved this to right after you create the datastore above and drop the "is_new".

Doing so would also alleviate a possible threading & datastore creating/add issue here too. As you have it now if two threads using the same channel enter this function then both could create and add the datastore. You probably want to create/add within the same "lock" scope.



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I01020769d91060a1f56a708eb405f87648d1a67e
Gerrit-Change-Number: 16231
Gerrit-PatchSet: 5
Gerrit-Owner: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Attention: N A <mail at interlinked.x10host.com>
Gerrit-Attention: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Mon, 09 Aug 2021 22:49:25 +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/20210809/53523b81/attachment.html>