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

[N A]

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

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


Patch Set 7:

(2 comments)

File funcs/func_scramble.c:

https://gerrit.asterisk.org/c/asterisk/+/16231/comment/7773510f_e2843ff0 
PS6, Line 195: 	ast_channel_lock(chan);
             : 	if (!(datastore = ast_channel_datastore_find(chan, &scramble_datastore, NULL))) {
             : 		ast_channel_unlock(chan);
             : 		/* Allocate a new datastore to hold the reference to this audiohook information */
             : 		if (!(datastore = ast_datastore_alloc(&scramble_datastore, NULL))) {
             : 			return 0;
             : 		}
             : 		if (!(ni = ast_calloc(1, sizeof(*ni)))) {
             : 			ast_datastore_free(datastore);
             : 			return 0;
             : 		}
             : 		ast_audiohook_init(&ni->audiohook, AST_AUDIOHOOK_TYPE_MANIPULATE, "Voice scrambler", AST_AUDIOHOOK_MANIPULATE_ALL_RATES);
             : 		ni->audiohook.manipulate_callback = scramble_callback;
             : 		datastore->data = ni;
             : 		ast_channel_lock(chan);
             : 		ast_channel_datastore_add(chan, datastore);
             : 		ast_audiohook_attach(chan, &ni->audiohook);
             : 	} else {
             : 		ni = datastore->data;
             : 	}
             : 	ast_channel_unlock(chan);
> Leave the lock/unlock calls surrounding the "if" but remove the ones embedded within. […]
Done - as a general rule of thumb, is this a best practice? In other existing functions setting up audiohooks, that seems to be the basic way they do it, i.e. the channel is not locked during the whole setup, e.g. func_pitchshift.c. So it's best to do it this way and this same issue would exist these other functions?


https://gerrit.asterisk.org/c/asterisk/+/16231/comment/ecd7eda9_7d0e8411 
PS6, Line 216: 	ni->tx = tx;
             : 	ni->rx = rx;
> Surround setting these variables too in the lock above otherwise two separate threads could attempt  […]
Done



-- 
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: 7
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: Kevin Harwell <kharwell at digium.com>
Gerrit-Attention: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Wed, 11 Aug 2021 17:48:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210811/22ff287f/attachment-0001.html>