[Asterisk-code-review] feat: AudioSocket channel and application (...asterisk[master])

[Joshua Colp]

Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/11579 )

Change subject: feat: AudioSocket channel and application
......................................................................


Patch Set 12:

(3 comments)

https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c 
File apps/app_audiosocket.c:

https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@156 
PS10, Line 156: 		if (!(f = ast_audiosocket_receive_frame(svc))) {
> I don't understand.  I'm freeing it in both the error and non-error conditions. […]
Within this function you are not freeing the frame returned by ast_audiosocket_receive_frame. That's what I was referring to. Frames should always be freed, or else you are depending on specific implementation knowledge to know that it is or is not necessary.


https://gerrit.asterisk.org/#/c/11579/10/channels/chan_audiosocket.c 
File channels/chan_audiosocket.c:

https://gerrit.asterisk.org/#/c/11579/10/channels/chan_audiosocket.c@136 
PS10, Line 136: 	struct audiosocket_instance *instance;
> I _think_ this addresses your concern (added NULL set to failure label), but I don't see any conditi […]
At line 172 you allocate this structure.
At line 184 is a failure case, this returns without freeing the above structure causing a leak. The same occurs at 185, 195, and 203.


https://gerrit.asterisk.org/#/c/11579/10/include/asterisk/res_audiosocket.h 
File include/asterisk/res_audiosocket.h:

https://gerrit.asterisk.org/#/c/11579/10/include/asterisk/res_audiosocket.h@84 
PS10, Line 84:  * \retval A \ref ast_frame on success
> Pardon my ignorance, but I would assume that would depend on the caller, no?  If it is processed int […]
Yes, that is my intention. APIs should be explicit in stating the required behavior so that an assumption isn't made, with an assumption the result can be a memory leak.



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Ie866e6c4fa13178ec76f2a6971ad3590a3a588b5
Gerrit-Change-Number: 11579
Gerrit-PatchSet: 12
Gerrit-Owner: Seán C. McCord <ulexus at gmail.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
Gerrit-Reviewer: Seán C. McCord <ulexus at gmail.com>
Gerrit-Comment-Date: Sat, 24 Aug 2019 11:40:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Seán C. McCord <ulexus at gmail.com>
Comment-In-Reply-To: Joshua Colp <jcolp at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190824/bfcf3f02/attachment-0001.html>