[Asterisk-code-review] ACN: Changes specific to the core (asterisk[master])

[Kevin Harwell]

Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/14688 )

Change subject: ACN: Changes specific to the core
......................................................................


Patch Set 2: Code-Review-1

(6 comments)

https://gerrit.asterisk.org/c/asterisk/+/14688/2/apps/app_dial.c 
File apps/app_dial.c:

https://gerrit.asterisk.org/c/asterisk/+/14688/2/apps/app_dial.c@1422 
PS2, Line 1422: 						if (f->data.ptr != NULL && f->datalen == sizeof(struct ast_stream_topology *)) {
There needs to be a better, more specific/deterministic way to distinguish the type of contained data.


https://gerrit.asterisk.org/c/asterisk/+/14688/2/include/asterisk/channel.h 
File include/asterisk/channel.h:

https://gerrit.asterisk.org/c/asterisk/+/14688/2/include/asterisk/channel.h@712 
PS2, Line 712: 	 * \since 18
Make this 18.0.0


https://gerrit.asterisk.org/c/asterisk/+/14688/2/main/channel.c 
File main/channel.c:

https://gerrit.asterisk.org/c/asterisk/+/14688/2/main/channel.c@1254 
PS2, Line 1254: 		.data.ptr = (void *) &topology,
              : 		.datalen = sizeof(topology)
I don't think we can rely on the caller to hold onto the topologies ref and pointer while it's passed through the core. It should be bumped here before sending.


https://gerrit.asterisk.org/c/asterisk/+/14688/2/main/frame.c 
File main/frame.c:

https://gerrit.asterisk.org/c/asterisk/+/14688/2/main/frame.c@143 
PS2, Line 143: 				&& fr->datalen == sizeof(struct ast_stream_topology *)) {
Same as in dial. A more robust way needs to be utilized in determining the type.


https://gerrit.asterisk.org/c/asterisk/+/14688/2/main/frame.c@144 
PS2, Line 144: 				struct ast_stream_topology *topology = *((struct ast_stream_topology **)fr->data.ptr);
> I'll let others chime in but it feels kludgey to use the data.ptr for this. […]
I agree with Josh here.


https://gerrit.asterisk.org/c/asterisk/+/14688/2/main/frame.c@222 
PS2, Line 222: 	if (fr->frametype == AST_FRAME_CONTROL && fr->subclass.integer == AST_CONTROL_ANSWER
             : 		&& fr->datalen == sizeof(struct ast_stream_topology *)) {
             : 		struct ast_stream_topology *topology = *((struct ast_stream_topology **)fr->data.ptr);
             : 		ao2_bump(topology);
             : 	}
I think if we're going to add the ability to pass ao2 objects through the framing core maybe  a more generic approach could be done. For instance when a frame is queued with data maybe a "cleanup" routine is also passed in that executes when the frame is done with it.



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I0115d2ed68d6bae0f87e85abcf16c771bdaf992c
Gerrit-Change-Number: 14688
Gerrit-PatchSet: 2
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Tue, 28 Jul 2020 18:39:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Joshua Colp <jcolp at sangoma.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200728/59d770fc/attachment-0001.html>