[Asterisk-code-review] bridge native rtp: Keep references to rtp instances on hook... (asterisk[13])

[George Joseph]

George Joseph has posted comments on this change. ( https://gerrit.asterisk.org/5854 )

Change subject: bridge_native_rtp:  Keep references to rtp instances on hook data
......................................................................


Patch Set 3:

(8 comments)

https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c
File bridges/bridge_native_rtp.c:

https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@47
PS3, Line 47: #include "asterisk/linkedlists.h"
> Why was this include added?
Done


https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@161
PS3, Line 161: 	AST_VECTOR_INIT(&data->instances, 4);
> AST_VECTOR_INIT() can fail to allocate the initial vector array.
no longer applicable


https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@207
PS3, Line 207: 	struct channel_instance_data *data = ao2_alloc(sizeof(*data),
             : 		channel_instance_data_destructor);
> Use ao2_alloc_options() since the object doesn't need any lock.
Done


https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@273
PS3, Line 273: 	RAII_VAR(struct ast_format_cap *, cap0,
             : 		ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT), ao2_cleanup);
             : 	RAII_VAR(struct ast_format_cap *, cap1,
             : 		ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT), ao2_cleanup);
> Pre-existing bug: cap0 and cap1 are not checked for failure.  Also allocati
Done


https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@301
PS3, Line 301: 		ast_debug(2, "Locally RTP bridged '%s' and '%s' in stack\n",
             : 			ast_channel_name(bc0->chan), ast_channel_name(bc1->chan));
             : 		ast_verb(4, "Locally RTP bridged '%s' and '%s' in stack\n",
             : 			ast_channel_name(bc0->chan), ast_channel_name(bc1->chan));
> The added debug log message is redundant with the existing verbose
 > message.  This is patch debugging and should not be left in the
 > final patch.

If you turn verbose on you get a ton of non-relevant messages.  By having the debus, you can just set debug 2 on bridge_native_rtp and see only the relevant messages.
They don't harm anything if dev mode isn't enabled.


https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@322
PS3, Line 322: 			ast_debug(2, "Remotely bridged '%s' and '%s' - media will flow directly between them\n",
             : 				ast_channel_name(bc0->chan), ast_channel_name(bc1->chan));
             : 			ast_verb(4, "Remotely bridged '%s' and '%s' - media will flow directly between them\n",
             : 				ast_channel_name(bc0->chan), ast_channel_name(bc1->chan));
> The added debug log message is redundant with the existing verbose message.
same


https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@740
PS3, Line 740: 	ao2_cleanup(data);
             : 	bridge_channel->tech_pvt = NULL;
> I'd swap these two lines for safety so the tech_pvt doesn't briefly
 > point to stale data.

no longer applicable


https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@781
PS3, Line 781: static void native_rtp_bridge_unsuspend(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel)
> A channel can get suspended from a bridge so it can play a sound file or it
Done



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

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e1ac49fa4af68d64826ccccd152593cf8cdb21a
Gerrit-Change-Number: 5854
Gerrit-PatchSet: 3
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Tue, 20 Jun 2017 19:49:30 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170620/591682b5/attachment.html>