[Asterisk-code-review] bridge softmix: Forward TEXT frames (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/6614 )

Change subject: bridge_softmix:  Forward TEXT frames
......................................................................


Patch Set 4: Code-Review-1

(5 comments)

https://gerrit.asterisk.org/#/c/6614/4/bridges/bridge_softmix.c
File bridges/bridge_softmix.c:

https://gerrit.asterisk.org/#/c/6614/4/bridges/bridge_softmix.c@683
PS4, Line 683: 	if (!caller || ast_strlen_zero(caller->id.name.str)) {
1) Use of caller->id.name.str also needs to be qualified by the caller->id.name.valid flag.

2) May also want to further check if the name is marked as private.

3) You should copy the name into a stack variable while bridge_channel->chan is locked.

4) caller can never be NULL since ast_channel_caller() returns an address inside the ast_channel struct.


https://gerrit.asterisk.org/#/c/6614/4/bridges/bridge_softmix.c@703
PS4, Line 703: 		ds = ast_channel_datastore_find(cur->chan, &text_message_originator_name, NULL);
It is easy to get the sender datastore out of sync with the message sender if you happen to have several people sending messages at the same time.


https://gerrit.asterisk.org/#/c/6614/4/bridges/bridge_softmix.c@707
PS4, Line 707: 			if (!(ds = ast_datastore_alloc(&text_message_originator_name, NULL))) {
Please get out of the bad habit of putting assignments in if statements.
1) They don't have good line breaks.
2) It is easy to get the isolation parentheses in the wrong place.


https://gerrit.asterisk.org/#/c/6614/4/bridges/bridge_softmix.c@713
PS4, Line 713: 		if (!(ds->data = ast_strdup(caller->id.name.str))) {
same


https://gerrit.asterisk.org/#/c/6614/4/channels/chan_pjsip.c
File channels/chan_pjsip.c:

https://gerrit.asterisk.org/#/c/6614/4/channels/chan_pjsip.c@2241
PS4, Line 2241: 	data->originator_name = NULL;
This assignment isn't necessary as ao2_alloc() has zeroed the struct.



-- 
To view, visit https://gerrit.asterisk.org/6614
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: Idacf5900bfd5f22ab8cd235aa56dfad090d18489
Gerrit-Change-Number: 6614
Gerrit-PatchSet: 4
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Thu, 28 Sep 2017 18:12:33 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170928/71b2bfc1/attachment-0001.html>