<p>Moritz Fain <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/9297">View Change</a></p><p>Patch set 1:</p><p>(7 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c">File res/res_stasis.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@521">Patch Set #1, Line 521:</a> <code style="font-family:monospace,monospace"> struct ast_channel *moh_channel = ast_channel_get_by_name(moh_wrapper->channel_id);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">For safety you need to check if moh_channel is NULL and unlink/unref moh_wr</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Correct. I will just return in case this is NULL. Possible but should not happen to often if the thread starts right away. Still possible :)</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@526">Patch Set #1, Line 526:</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;"> ao2_lock(app_bridges_moh);<br> ao2_unlink_flags(app_bridges_moh, moh_wrapper, OBJ_NOLOCK);<br><br> ast_moh_stop(moh_channel);<br> ast_hangup(moh_channel);<br><br> ao2_unlock(app_bridges_moh);<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">You just need to unlink moh_wrapper from the container and unref moh_wrappe</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yes!</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@567">Patch Set #1, Line 567:</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;"> /* The after bridge callback assumes responsibility of the bridge_id. */<br> if (ast_bridge_set_after_callback(chan,<br> moh_after_bridge_cb, moh_after_bridge_cb_failed, bridge_id)) {<br> ast_hangup(chan);<br> return NULL;<br> }<br> bridge_id = NULL;<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This is dead code and needs to be removed along with the passed in callback</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">When are these callbacks going to be called? Sounds like when the bridge is destroyed. And the callback removes the MOH announcer channel. Not to bad?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Is this called AFTER the threads exits? Then it might be better to do the cleanup here rather than in the thread?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@588">Patch Set #1, Line 588:</a> <code style="font-family:monospace,monospace"> if (ast_string_field_init(new_wrapper, 32)) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">The initial size should be increased as we are likely to always need more s</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yes</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@588">Patch Set #1, Line 588:</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_string_field_init(new_wrapper, 32)) {<br> ast_hangup(chan);<br> return NULL;<br> }<br> ast_string_field_set(new_wrapper, bridge_id, bridge->uniqueid);<br> ast_string_field_set(new_wrapper, channel_id, ast_channel_uniqueid(chan));<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Since we are passing new_wrapper to moh_channel_thread() these string field</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yes</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@600">Patch Set #1, Line 600:</a> <code style="font-family:monospace,monospace"> if (ast_pthread_create_detached(&threadid, NULL, moh_channel_thread, new_wrapper)) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">We need to pass a new_wrapper ref to moh_channel_thread(). We cannot rely </blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ok, will fix that. I'm not deep enough into ao2 stuff.<br>Basically it's just updating the ref counter so that the wrapper will not get de-allocated?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@625">Patch Set #1, Line 625:</a> <code style="font-family:monospace,monospace"> ao2_unlock(app_bridges_moh);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Moving this lock doesn't help. As soon as you release the lock someone cou</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">It's not about locking the channel. It's about locking the wrapper.</p><p style="white-space: pre-wrap; word-wrap: break-word;">If I move the unlock where it was I end up having a moh_wrapper structure (in line 621) whose channel might not be there any more.</p><p style="white-space: pre-wrap; word-wrap: break-word;">That's why I moved the unlock at the end.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/9297">change 9297</a>. To unsubscribe, 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/9297"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 15 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Ibff479e167b3320c68aaabfada7e1d0ef7bd548c </div>
<div style="display:none"> Gerrit-Change-Number: 9297 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Moritz Fain <moritz@fain.io> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Moritz Fain <moritz@fain.io> </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 27 Jun 2018 10:38:38 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>