<p>Patch set 10:<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>21 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11579/10/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/10/apps/app_audiosocket.c@30">Patch Set #10, Line 30:</a> <code style="font-family:monospace,monospace">   <support_level>core</support_level></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should be extended</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@75">Patch Set #10, Line 75:</a> <code style="font-family:monospace,monospace">static int audiosocket_exec(struct ast_channel *chan, const char *data);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This doesn't need to be forward declared.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@85">Patch Set #10, Line 85:</a> <code style="font-family:monospace,monospace">    );</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Indentation</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@102">Patch Set #10, Line 102:</a> <code style="font-family:monospace,monospace">           ast_log(LOG_ERROR, "failed to connect to AudioSocket\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should this cause the application to exit?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@113">Patch Set #10, Line 113:</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;"><br>     if (ast_set_write_format(chan, 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(chan, 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;">The original write and read formats should be stored and then restored when this is completed.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@131">Patch Set #10, Line 131:</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_channel_state(chan) != AST_STATE_UP) {<br>                        return 0;<br>             }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should this be done when this function is entered?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@137">Patch Set #10, Line 137:</a> <code style="font-family:monospace,monospace">                  ast_log(LOG_WARNING, "No frame received\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This will occur under normal conditions if the channel is hung up</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@141">Patch Set #10, Line 141:</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;">     f->delivery.tv_sec = 0;<br>            f->delivery.tv_usec = 0;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why is this done?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@156">Patch Set #10, Line 156:</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 this frame be 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/10/apps/app_audiosocket.c@183">Patch Set #10, Line 183:</a> <code style="font-family:monospace,monospace">    .support_level = AST_MODULE_SUPPORT_CORE,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">EXTENDED</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11579/10/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/10/channels/chan_audiosocket.c@30">Patch Set #10, Line 30:</a> <code style="font-family:monospace,monospace">       <support_level>core</support_level></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">extended</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/10/channels/chan_audiosocket.c@136">Patch Set #10, Line 136:</a> <code style="font-family:monospace,monospace">     struct audiosocket_instance *instance;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">In certain off-nominal error cases this can end up getting leaked</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/10/channels/chan_audiosocket.c@260">Patch Set #10, Line 260:</a> <code style="font-family:monospace,monospace">    .support_level = AST_MODULE_SUPPORT_CORE,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">EXTENDED</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11579/10/include/asterisk/res_audiosocket.h">File include/asterisk/res_audiosocket.h:</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/10/include/asterisk/res_audiosocket.h@74">Patch Set #10, Line 74:</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;"> * This returned object is an ao2 reference counted object.<br> *<br> * Any attribute in the returned \ref hepv3_capture_info that is a<br> * pointer should point to something that is allocated on the heap,<br> * as it will be free'd when the \ref hepv3_capture_info object is<br> * reclaimed.<br> *<br> * \param payload The payload to send to the HEP capture node<br> * \param len     Length of \ref payload<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This isn't hep</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/10/include/asterisk/res_audiosocket.h@84">Patch Set #10, Line 84:</a> <code style="font-family:monospace,monospace"> * \retval A \ref ast_frame on success</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">What should be done with the returned frame? Does it need ast_frfree called on it?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11579/10/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/10/res/res_audiosocket.c@28">Patch Set #10, Line 28:</a> <code style="font-family:monospace,monospace"> <support_level>core</support_level></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">extended</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/10/res/res_audiosocket.c@55">Patch Set #10, Line 55:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If this is called from a channel thread and either resolution or connection takes awhile to succeed/fail, data can then pile up on the underlying source that is feeding the channel (such as the RTP socket). It's probably wise to put the channel into autoservice before calling this so that doesn't occur.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/10/res/res_audiosocket.c@106">Patch Set #10, Line 106:</a> <code style="font-family:monospace,monospace"> * \brief Handle the connection that was started by launch_netscript.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Doesn't match how this is used</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/10/res/res_audiosocket.c@174">Patch Set #10, Line 174:</a> <code style="font-family:monospace,monospace">  usleep(100);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why this usleep?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/10/res/res_audiosocket.c@196">Patch Set #10, Line 196:</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;">I don't believe you need to malloc this. I'd expect the stack would be fine.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11579/10/res/res_audiosocket.c@307">Patch Set #10, Line 307:</a> <code style="font-family:monospace,monospace"> return &f;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Can't this be called from multiple threads at the same time, thus resulting in this same frame being used by each since it is static?</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: 10 </div>
<div style="display:none"> Gerrit-Owner: Seán C. McCord <ulexus@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Sean Bright <sean.bright@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Seán C. McCord <ulexus@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 20 Aug 2019 12:40:32 +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>