<blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Patch Set 12: Code-Review-1</p><p style="white-space: pre-wrap; word-wrap: break-word;">(1 comment)</p><p style="white-space: pre-wrap; word-wrap: break-word;">Is UUID absolutely required?  Could you just use the unique channel id, whether auto-generated or provided in an ARI call?   If so, then chan_audiosocket could easily plug into the new externalMedia thing with encapsulation "audiosocket" and transport "tcp". with no other changes to your code.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">By the current protocol specification, it is a hard requirement, but I an not averse modifying that spec.  UUID is nice because it is a fixed-length binary ID, but beyond that, it is completely arbitrary.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I'll see what will need to be changed for this and how disruptive it might be.<br></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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This should be extended</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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@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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This doesn't need to be forward declared.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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@85">Patch Set #10, Line 85:</a> <code style="font-family:monospace,monospace">    );</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Indentation</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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@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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should this cause the application to exit?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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@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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">The original write and read formats should be stored and then restored when this is completed.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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@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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should this be done when this function is entered?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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@137">Patch Set #10, Line 137:</a> <code style="font-family:monospace,monospace">                      ast_log(LOG_WARNING, "No frame received\n");</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This will occur under normal conditions if the channel is hung up</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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@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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why is this done?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should this frame be freed?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't understand.  I'm freeing it in both the error and non-error conditions.  Are you suggesting that the frame should _not_ be freed?  Is that because I subsequently reuse the variable for the receive-from-audiosocket and thus, it is redundant?  I was assuming there may be inner structs which ast_frfree() would properly dispose of which may not be, should I just reuse the pointer.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">EXTENDED</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">extended</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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/channels/chan_audiosocket.c@136">Patch Set #10, Line 136:</a> <code style="font-family:monospace,monospace"> struct audiosocket_instance *instance;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">In certain off-nominal error cases this can end up getting leaked</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I _think_ this addresses your concern (added NULL set to failure label), but I don't see any condition in which that would have happened.  If there some other error-related callback I should be setting on the module to handle this?</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">EXTENDED</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This isn't hep</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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/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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">What should be done with the returned frame? Does it need ast_frfree called on it?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Pardon my ignorance, but I would assume that would depend on the caller, no?  If it is processed internally, it would be freed.  If it is passed on down through a channel, it would be handled by whatever handler is down the line as per usual.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I have added a note in the parameter suggesting that freeing the frame is the responsibility of the caller.  Is this your intention?</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">extended</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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/res/res_audiosocket.c@55">Patch Set #10, Line 55:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">If this is called from a channel thread and either resolution or connection takes awhile to succeed/ […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Nice; I didn't know about autoservice.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Doesn't match how this is used</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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/res/res_audiosocket.c@174">Patch Set #10, Line 174:</a> <code style="font-family:monospace,monospace">      usleep(100);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why this usleep?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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/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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I don't believe you need to malloc this. I'd expect the stack would be fine.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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/res/res_audiosocket.c@307">Patch Set #10, Line 307:</a> <code style="font-family:monospace,monospace">     return &f;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Can't this be called from multiple threads at the same time, thus resulting in this same frame being […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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: 12 </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: George Joseph <gjoseph@digium.com> </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: Sat, 24 Aug 2019 01:49:10 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>