<p style="white-space: pre-wrap; word-wrap: break-word;">Still working through making all the changes</p><p><a href="https://gerrit.asterisk.org/10937">View Change</a></p><p>5 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10937/6/res/res_stasis.c">File res/res_stasis.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/10937/6/res/res_stasis.c@1416">Patch Set #6, Line 1416:</a> <code style="font-family:monospace,monospace"> break;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">msg and next_app are ref leaked here</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think these are both ok. msg will still be unref'd whether there was an error or not, and I've added an ao2_cleanup after control_move_cleanup which should take care of next_app in the case of it not being active but having a ref.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10937/6/res/res_stasis.c@1419">Patch Set #6, Line 1419:</a> <code style="font-family:monospace,monospace"> if (r == 0) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I don't think you need this check since if "r" is ever non-zero it breaks out of the loop above.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think it needs to be there, strictly for that error scenario. If "r" is non-zero (breaking the loop), then something went wrong and the message shouldn't be sent. It will only ever be 0 here if it makes it through the entire loop successfully.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.h">File res/stasis/control.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/10937/6/res/stasis/control.h@40">Patch Set #6, Line 40:</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> * \note This function inherits app's ref rather than bumping app!<br> * If something goes wrong, ao2_cleanup is called on the app.<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Any particular reason for this change? To me it breaks the usual expectations a bit as we typically […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The main reason was to do away with having to have a variable named "app" inside of the stasis_app_exec function. It was relying on using it and "app_name" to do all function calls, when it should have been using "control_xxx" functions to get the data. This is more important now since the application can change within the control structure. The reason it now inherits the ref is because when the function is called to create the control, the parameter for "app" is passed in as an "ao2_find" and we don't actually have direct access to the app without first creating the control.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.c">File res/stasis/control.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/10937/6/res/stasis/control.c@86">Patch Set #6, Line 86:</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> * The list of arguments to pass to StasisStart when moving to another app.<br> */<br> char **argv;<br> /*!<br> * The number of arguments argv contains.<br> */<br> int argc;<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I'm thinking you can just use a string vector here instead. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Posting what we talked about in stand-up for public visibility: stasis start requires a parameter of type "char **" for the argument list, and if using a vector, we would need to access the raw buffer, which technically "violates" the way vectors are supposed to be used.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.c@129">Patch Set #6, Line 129:</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;"> res = ast_cond_init(&control->wait_cond, NULL);<br> if (res != 0) {<br> ast_log(LOG_ERROR, "Error initializing ast_cond_t: %s\n",<br> strerror(errno));<br> ao2_cleanup(app);<br> ao2_ref(control, -1);<br> return NULL;<br> }<br><br> control->app = app;<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I'm not a fan of stealing the ref here, but if you do it this way you could move this just before th […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I can move it up, but will wait on feedback from reasoning behind the stolen ref.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/10937">change 10937</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/10937"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I43d12b10045a98a8d42541889b85695be26f288a </div>
<div style="display:none"> Gerrit-Change-Number: 10937 </div>
<div style="display:none"> Gerrit-PatchSet: 6 </div>
<div style="display:none"> Gerrit-Owner: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation (1000185) </div>
<div style="display:none"> Gerrit-Reviewer: Joshua C. Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 25 Feb 2019 17:10:52 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>