<p>Patch set 19:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/11579">View Change</a></p><p>22 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11579/19/apps/app_audiosocket.c">File apps/app_audiosocket.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/19/apps/app_audiosocket.c@110">Patch Set #19, Line 110:</a> <code style="font-family:monospace,monospace"> writeFormat = ast_channel_writeformat(chan);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/19/apps/app_audiosocket.c@113">Patch Set #19, Line 113:</a> <code style="font-family:monospace,monospace">            ast_log(LOG_ERROR, "Failed to set write format to SLINEAR\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">These and below log messages should include the channel name</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/19/apps/app_audiosocket.c@128">Patch Set #19, Line 128:</a> <code style="font-family:monospace,monospace">     audiosocket_run(chan, args.idStr, s);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">None of the return values are handled here, should they be?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/19/apps/app_audiosocket.c@132">Patch Set #19, Line 132:</a> <code style="font-family:monospace,monospace">         ast_log(LOG_ERROR, "Failed to store write format\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">restore</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/19/apps/app_audiosocket.c@135">Patch Set #19, Line 135:</a> <code style="font-family:monospace,monospace">           ast_log(LOG_ERROR, "Failed to store read format\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">restore</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/19/apps/app_audiosocket.c@156">Patch Set #19, Line 156:</a> <code style="font-family:monospace,monospace">                    return 0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/19/apps/app_audiosocket.c@163">Patch Set #19, Line 163:</a> <code style="font-family:monospace,monospace">                            ast_log(LOG_ERROR, "Failed to forward channel frame to AudioSocket\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">channel name, including others</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/19/apps/app_audiosocket.c@172">Patch Set #19, Line 172:</a> <code style="font-family:monospace,monospace">          if (!(f = ast_audiosocket_receive_frame(svc))) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should the file descriptor also be polled on so this can be called whenever traffic is received instead of just always here?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11579/19/channels/chan_audiosocket.c">File channels/chan_audiosocket.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/19/channels/chan_audiosocket.c@47">Patch Set #19, Line 47:</a> <code style="font-family:monospace,monospace">  int svc;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think this needs documentation of what these are for.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/19/channels/chan_audiosocket.c@76">Patch Set #19, Line 76:</a> <code style="font-family:monospace,monospace">       if (!ast) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Are there actually cases where this happens? That would be a violation of the APIs.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/19/channels/chan_audiosocket.c@81">Patch Set #19, Line 81:</a> <code style="font-family:monospace,monospace">        if (instance == NULL || instance->svc < 1) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">a #define with the meaning of 1 would be nice</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/19/channels/chan_audiosocket.c@110">Patch Set #19, Line 110:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if (ast_set_write_format(ast, ast_format_slin)) {<br>             ast_log(LOG_ERROR, "Failed to set write format to SLINEAR\n");<br>              return -1;<br>    }<br>     if (ast_set_read_format(ast, ast_format_slin)) {<br>              ast_log(LOG_ERROR, "Failed to set read format to SLINEAR\n");<br>               return -1;<br>    }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/19/channels/chan_audiosocket.c@137">Patch Set #19, Line 137:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Where is the instance actually freed?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/19/channels/chan_audiosocket.c@191">Patch Set #19, Line 191:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if (ast_format_cap_iscompatible_format(cap, ast_format_slin) == AST_FORMAT_CMP_NOT_EQUAL) {<br>            struct ast_str *cap_buf = ast_str_alloca(AST_FORMAT_CAP_NAMES_LEN);<br>           if (cap_buf < 0) {<br>                 ast_log(LOG_ERROR, "Failed to allocate AudioSocket capabilities buffer\n");<br>                 goto failure;<br>         }<br><br>           ast_log(LOG_NOTICE, "Asked to get a channel of unsupported format '%s'\n",<br>                  ast_format_cap_get_names(cap, &cap_buf));<br>         goto failure;<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Since cap isn't actually used to influence things I don't think this is actually needed.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/19/channels/chan_audiosocket.c@232">Patch Set #19, Line 232:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  if (instance != NULL) {<br>               ast_free(instance);<br>   }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11579/19/res/res_audiosocket.c">File res/res_audiosocket.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/19/res/res_audiosocket.c@118">Patch Set #19, Line 118:</a> <code style="font-family:monospace,monospace">          ast_log(LOG_ERROR, "Failed to resolve AudioSocket service");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">What is being resolved should be logged here</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/19/res/res_audiosocket.c@126">Patch Set #19, Line 126:</a> <code style="font-family:monospace,monospace">                        ast_log(LOG_WARNING, "No port provided");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/19/res/res_audiosocket.c@204">Patch Set #19, Line 204:</a> <code style="font-family:monospace,monospace">   if (!(buf = ast_malloc(3 + f->datalen))) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Can't this just be declared:</p><p style="white-space: pre-wrap; word-wrap: break-word;">uint8_t buf[3 + f->datalen];</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/19/res/res_audiosocket.c@300">Patch Set #19, Line 300:</a> <code style="font-family:monospace,monospace">           return NULL;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">data leak</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/19/res/res_audiosocket.c@304">Patch Set #19, Line 304:</a> <code style="font-family:monospace,monospace">             return &ast_null_frame;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">data leak</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/19/res/res_audiosocket.c@314">Patch Set #19, Line 314:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">At the end I'd add a comment stating that the isolated frame steals the data so it doesn't need to be freed</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11579/19/res/res_audiosocket.exports.in">File res/res_audiosocket.exports.in:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/19/res/res_audiosocket.exports.in@6">Patch Set #19, Line 6:</a> <code style="font-family:monospace,monospace">          LINKER_SYMBOL_PREFIX*ast_audiosocket_receive_frame;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why the * here?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/11579">change 11579</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/11579"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Ie866e6c4fa13178ec76f2a6971ad3590a3a588b5 </div>
<div style="display:none"> Gerrit-Change-Number: 11579 </div>
<div style="display:none"> Gerrit-PatchSet: 19 </div>
<div style="display:none"> Gerrit-Owner: Seán C. McCord <ulexus@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Seán C. McCord <ulexus@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 09 Oct 2019 13:40:06 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>