<p>George Joseph <strong>merged</strong> this change.</p><p><a href="https://gerrit.asterisk.org/10379">View Change</a></p><div style="white-space:pre-wrap">Approvals:
George Joseph: Looks good to me, but someone else must approve; Approved for Submit
Corey Farrell: Looks good to me, but someone else must approve
Joshua Colp: Looks good to me, approved
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">res_smdi.c: Fix module ref counting and inverted test.<br><br>I think this module is so screwed up that it doesn't work anymore. Even<br>with these attempts to fix things it still won't gracefully shut down.<br>The module refs will not go to zero to allow unloading the module.<br><br>* Fix module ref counting dealing with the SMDI interface object. There<br>were several off-nominal paths that unbalanced the module ref count. Also<br>the destructor freed the ao2 object itself which is bad. Made the<br>smdi_read thread not hold its own ref to the SMDI interface object so when<br>all refs go away the destructor will stop the listener thread.<br><br>* Fixed the smdi_load() return code of 1 concerning the number of<br>listeners. The test was inverted.<br><br>Change-Id: Ic288db51b58e395d6a2fc3847f77176c16988784<br>---<br>M res/res_smdi.c<br>1 file changed, 18 insertions(+), 12 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/res/res_smdi.c b/res/res_smdi.c</span><br><span>index 8622da4..1232839 100644</span><br><span>--- a/res/res_smdi.c</span><br><span>+++ b/res/res_smdi.c</span><br><span>@@ -226,10 +226,12 @@</span><br><span> static void smdi_interface_destroy(void *obj)</span><br><span> {</span><br><span> struct ast_smdi_interface *iface = obj;</span><br><span style="color: hsl(120, 100%, 40%);">+ int mod_unref_defer = 0;</span><br><span> </span><br><span> if (iface->thread != AST_PTHREADT_NULL && iface->thread != AST_PTHREADT_STOP) {</span><br><span> pthread_cancel(iface->thread);</span><br><span> pthread_join(iface->thread, NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ mod_unref_defer = 1;</span><br><span> }</span><br><span> </span><br><span> iface->thread = AST_PTHREADT_STOP;</span><br><span>@@ -246,9 +248,9 @@</span><br><span> ast_mutex_destroy(&iface->mwi_q_lock);</span><br><span> ast_cond_destroy(&iface->mwi_q_cond);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- free(iface);</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">- ast_module_unref(ast_module_info->self);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (mod_unref_defer) {</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_module_unref(ast_module_info->self);</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span> }</span><br><span> </span><br><span> /*!</span><br><span>@@ -611,7 +613,6 @@</span><br><span> </span><br><span> md_msg = ao2_alloc(sizeof(*md_msg), NULL);</span><br><span> if (!md_msg) {</span><br><span style="color: hsl(0, 100%, 40%);">- ao2_ref(iface, -1);</span><br><span> return NULL;</span><br><span> }</span><br><span> </span><br><span>@@ -621,7 +622,6 @@</span><br><span> if (c == EOF) {</span><br><span> ast_log(LOG_ERROR, "Unexpected EOF while reading MD message\n");</span><br><span> ao2_ref(md_msg, -1);</span><br><span style="color: hsl(0, 100%, 40%);">- ao2_ref(iface, -1);</span><br><span> return NULL;</span><br><span> }</span><br><span> md_msg->mesg_desk_num[i] = (char) c;</span><br><span>@@ -638,7 +638,6 @@</span><br><span> if (c == EOF) {</span><br><span> ast_log(LOG_ERROR, "Unexpected EOF while reading SMDI message\n");</span><br><span> ao2_ref(md_msg, -1);</span><br><span style="color: hsl(0, 100%, 40%);">- ao2_ref(iface, -1);</span><br><span> return NULL;</span><br><span> }</span><br><span> md_msg->mesg_desk_term[i] = (char) c;</span><br><span>@@ -654,7 +653,6 @@</span><br><span> if (c == EOF) {</span><br><span> ast_log(LOG_ERROR, "Unexpected EOF while reading SMDI message\n");</span><br><span> ao2_ref(md_msg, -1);</span><br><span style="color: hsl(0, 100%, 40%);">- ao2_ref(iface, -1);</span><br><span> return NULL;</span><br><span> }</span><br><span> md_msg->type = (char) c;</span><br><span>@@ -733,7 +731,6 @@</span><br><span> </span><br><span> mwi_msg = ao2_alloc(sizeof(*mwi_msg), NULL);</span><br><span> if (!mwi_msg) {</span><br><span style="color: hsl(0, 100%, 40%);">- ao2_ref(iface, -1);</span><br><span> return NULL;</span><br><span> }</span><br><span> </span><br><span>@@ -767,7 +764,6 @@</span><br><span> if (c == EOF) {</span><br><span> ast_log(LOG_ERROR, "Unexpected EOF while reading MWI message\n");</span><br><span> ao2_ref(mwi_msg, -1);</span><br><span style="color: hsl(0, 100%, 40%);">- ao2_ref(iface, -1);</span><br><span> return NULL;</span><br><span> }</span><br><span> mwi_msg->cause[i] = (char) c;</span><br><span>@@ -788,7 +784,6 @@</span><br><span> }</span><br><span> </span><br><span> ast_log(LOG_ERROR, "Error reading from SMDI interface %s, stopping listener thread\n", iface->name);</span><br><span style="color: hsl(0, 100%, 40%);">- ao2_ref(iface, -1);</span><br><span> return NULL;</span><br><span> }</span><br><span> </span><br><span>@@ -985,6 +980,11 @@</span><br><span> return 0;</span><br><span> </span><br><span> new_ifaces = ao2_container_alloc_list(AO2_ALLOC_OPT_LOCK_MUTEX, 0, NULL, smdi_ifaces_cmp_fn);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!new_ifaces) {</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_config_destroy(conf);</span><br><span style="color: hsl(120, 100%, 40%);">+ return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> for (v = ast_variable_browse(conf, "interfaces"); v; v = v->next) {</span><br><span> RAII_VAR(struct ast_smdi_interface *, iface, NULL, ao2_cleanup);</span><br><span> </span><br><span>@@ -1107,7 +1107,13 @@</span><br><span> /* set the message expiry time */</span><br><span> iface->msg_expiry = msg_expiry;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- /* start the listener thread */</span><br><span style="color: hsl(120, 100%, 40%);">+ /*</span><br><span style="color: hsl(120, 100%, 40%);">+ * start the listener thread</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * The listener thread does not actually hold a ref to iface. When all</span><br><span style="color: hsl(120, 100%, 40%);">+ * external refs go away, the destructor will stop the listener thread</span><br><span style="color: hsl(120, 100%, 40%);">+ * before actually destroying the iface object.</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span> ast_verb(3, "Starting SMDI monitor thread for %s\n", iface->name);</span><br><span> if (ast_pthread_create_background(&iface->thread, NULL, smdi_read, iface)) {</span><br><span> ast_log(LOG_ERROR, "Error starting SMDI monitor thread for %s\n", iface->name);</span><br><span>@@ -1157,7 +1163,7 @@</span><br><span> return -1;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (ao2_container_count(new_ifaces)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!ao2_container_count(new_ifaces)) {</span><br><span> res = 1;</span><br><span> }</span><br><span> </span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/10379">change 10379</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/10379"/><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: merged </div>
<div style="display:none"> Gerrit-Change-Id: Ic288db51b58e395d6a2fc3847f77176c16988784 </div>
<div style="display:none"> Gerrit-Change-Number: 10379 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 (1000185) </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>