[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 19: Code-Review-1

(22 comments)

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

https://gerrit.asterisk.org/#/c/11579/19/apps/app_audiosocket.c@110 
PS19, Line 110: 	writeFormat = ast_channel_writeformat(chan);
Both writeFormat and readFormat need to have a reference bumped on them, and then decreased later. This is because once ast_set_write_format and ast_set_read_format are called they could go away if non-cached.


https://gerrit.asterisk.org/#/c/11579/19/apps/app_audiosocket.c@113 
PS19, Line 113: 		ast_log(LOG_ERROR, "Failed to set write format to SLINEAR\n");
These and below log messages should include the channel name


https://gerrit.asterisk.org/#/c/11579/19/apps/app_audiosocket.c@128 
PS19, Line 128: 	audiosocket_run(chan, args.idStr, s);
None of the return values are handled here, should they be?


https://gerrit.asterisk.org/#/c/11579/19/apps/app_audiosocket.c@132 
PS19, Line 132: 		ast_log(LOG_ERROR, "Failed to store write format\n");
restore


https://gerrit.asterisk.org/#/c/11579/19/apps/app_audiosocket.c@135 
PS19, Line 135: 		ast_log(LOG_ERROR, "Failed to store read format\n");
restore


https://gerrit.asterisk.org/#/c/11579/19/apps/app_audiosocket.c@156 
PS19, Line 156: 			return 0;
If a frame can't be read this is due to the channel being hung up generally, and a -1 should get bubbled up from the application to the PBX core so execution does not continue.


https://gerrit.asterisk.org/#/c/11579/19/apps/app_audiosocket.c@163 
PS19, Line 163: 				ast_log(LOG_ERROR, "Failed to forward channel frame to AudioSocket\n");
channel name, including others


https://gerrit.asterisk.org/#/c/11579/19/apps/app_audiosocket.c@172 
PS19, Line 172: 		if (!(f = ast_audiosocket_receive_frame(svc))) {
Should the file descriptor also be polled on so this can be called whenever traffic is received instead of just always here?


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

https://gerrit.asterisk.org/#/c/11579/19/channels/chan_audiosocket.c@47 
PS19, Line 47: 	int svc;
I think this needs documentation of what these are for.


https://gerrit.asterisk.org/#/c/11579/19/channels/chan_audiosocket.c@76 
PS19, Line 76: 	if (!ast) {
Are there actually cases where this happens? That would be a violation of the APIs.


https://gerrit.asterisk.org/#/c/11579/19/channels/chan_audiosocket.c@81 
PS19, Line 81: 	if (instance == NULL || instance->svc < 1) {
a #define with the meaning of 1 would be nice


https://gerrit.asterisk.org/#/c/11579/19/channels/chan_audiosocket.c@110 
PS19, Line 110: if (ast_set_write_format(ast, ast_format_slin)) {
              : 		ast_log(LOG_ERROR, "Failed to set write format to SLINEAR\n");
              : 		return -1;
              : 	}
              : 	if (ast_set_read_format(ast, ast_format_slin)) {
              : 		ast_log(LOG_ERROR, "Failed to set read format to SLINEAR\n");
              : 		return -1;
              : 	}
Past channel creation in audiosocket_request these should not be touched. Control of these go to whatever is handling the channel, as this controls what they want to read and write for format.


https://gerrit.asterisk.org/#/c/11579/19/channels/chan_audiosocket.c@137 
PS19, Line 137: 
Where is the instance actually freed?


https://gerrit.asterisk.org/#/c/11579/19/channels/chan_audiosocket.c@191 
PS19, Line 191: if (ast_format_cap_iscompatible_format(cap, ast_format_slin) == AST_FORMAT_CMP_NOT_EQUAL) {
              : 		struct ast_str *cap_buf = ast_str_alloca(AST_FORMAT_CAP_NAMES_LEN);
              : 		if (cap_buf < 0) {
              : 			ast_log(LOG_ERROR, "Failed to allocate AudioSocket capabilities buffer\n");
              : 			goto failure;
              : 		}
              : 
              : 		ast_log(LOG_NOTICE, "Asked to get a channel of unsupported format '%s'\n",
              : 			ast_format_cap_get_names(cap, &cap_buf));
              : 		goto failure;
              : 	}
Since cap isn't actually used to influence things I don't think this is actually needed.


https://gerrit.asterisk.org/#/c/11579/19/channels/chan_audiosocket.c@232 
PS19, Line 232: 	if (instance != NULL) {
              : 		ast_free(instance);
              : 	}
I don't understand this... instance is set on the channel, but then is freed here? Did an "instance = NULL;" get lost? If added back in a comment about it would be good.


https://gerrit.asterisk.org/#/c/11579/19/res/res_audiosocket.c 
File res/res_audiosocket.c:

https://gerrit.asterisk.org/#/c/11579/19/res/res_audiosocket.c@118 
PS19, Line 118: 		ast_log(LOG_ERROR, "Failed to resolve AudioSocket service");
What is being resolved should be logged here


https://gerrit.asterisk.org/#/c/11579/19/res/res_audiosocket.c@126 
PS19, Line 126: 			ast_log(LOG_WARNING, "No port provided");
I think this should include the address, and since you can only pass in a single hostname wouldn't all resolved addresses have the same problem?


https://gerrit.asterisk.org/#/c/11579/19/res/res_audiosocket.c@204 
PS19, Line 204: 	if (!(buf = ast_malloc(3 + f->datalen))) {
Can't this just be declared:

uint8_t buf[3 + f->datalen];


https://gerrit.asterisk.org/#/c/11579/19/res/res_audiosocket.c@300 
PS19, Line 300: 		return NULL;
data leak


https://gerrit.asterisk.org/#/c/11579/19/res/res_audiosocket.c@304 
PS19, Line 304: 		return &ast_null_frame;
data leak


https://gerrit.asterisk.org/#/c/11579/19/res/res_audiosocket.c@314 
PS19, Line 314: 
At the end I'd add a comment stating that the isolated frame steals the data so it doesn't need to be freed


https://gerrit.asterisk.org/#/c/11579/19/res/res_audiosocket.exports.in 
File res/res_audiosocket.exports.in:

https://gerrit.asterisk.org/#/c/11579/19/res/res_audiosocket.exports.in@6 
PS19, Line 6: 		LINKER_SYMBOL_PREFIX*ast_audiosocket_receive_frame;
Why the * 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: 19
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 digium.com>
Gerrit-Reviewer: Seán C. McCord <ulexus at gmail.com>
Gerrit-Comment-Date: Wed, 09 Oct 2019 13:40:06 +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/20191009/3ad04f76/attachment-0001.html>