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

[Kevin Harwell]

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

Change subject: feat: AudioSocket channel, application, and ARI support.
......................................................................


Patch Set 33: Code-Review-1

(6 comments)

https://gerrit.asterisk.org/c/asterisk/+/11579/33/apps/app_audiosocket.c 
File apps/app_audiosocket.c:

https://gerrit.asterisk.org/c/asterisk/+/11579/33/apps/app_audiosocket.c@115 
PS33, Line 115: 		return -1;
The audio socket needs to be closed before returning here.


https://gerrit.asterisk.org/c/asterisk/+/11579/33/apps/app_audiosocket.c@128 
PS33, Line 128: 		return -1;
The audio socket needs to be closed before returning here.


https://gerrit.asterisk.org/c/asterisk/+/11579/33/doc/CHANGES-staging/feat_audiosocket.txt 
File doc/CHANGES-staging/feat_audiosocket.txt:

https://gerrit.asterisk.org/c/asterisk/+/11579/33/doc/CHANGES-staging/feat_audiosocket.txt@4 
PS33, Line 4: applciation
s/applciation/application


https://gerrit.asterisk.org/c/asterisk/+/11579/33/res/res_audiosocket.c 
File res/res_audiosocket.c:

https://gerrit.asterisk.org/c/asterisk/+/11579/33/res/res_audiosocket.c@149 
PS33, Line 149: 		} else {
              : 			ast_log(LOG_ERROR, "Connection to %s failed with unexpected error: %s\n",
              : 				ast_sockaddr_stringify(&addrs[i]), strerror(errno));
              : 		}
I think the connection needs to be closed here, and set to -1. Otherwise it's possible this function can return with a non connected socket.


https://gerrit.asterisk.org/c/asterisk/+/11579/33/res/res_audiosocket.c@162 
PS33, Line 162: 	if (chan && ast_autoservice_stop(chan) < 0) {
              : 		ast_log(LOG_WARNING, "Failed to stop autoservice for channel %s\n",
              : 		ast_channel_name(chan));
              : 		return -1;
              : 	}
Should we close the socket in this case too? It might be okay to continue in this case. If so then this should return 's' and not -1.


https://gerrit.asterisk.org/c/asterisk/+/11579/33/res/res_audiosocket.c@167 
PS33, Line 167: 
              : 	if (i == num_addrs) {
              : 		ast_log(LOG_ERROR, "Failed to connect to AudioSocket service\n");
              : 		return -1;
              : 	}
This is an error, so I'd say close the socket before returning here.



-- 
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: 33
Gerrit-Owner: Seán C. McCord <ulexus at gmail.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Seán C. McCord <ulexus at gmail.com>
Gerrit-Comment-Date: Fri, 10 Jan 2020 15:57:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200110/56473709/attachment-0001.html>