<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/3184/">https://reviewboard.asterisk.org/r/3184/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 6th, 2014, 1:21 p.m. MST, <b>rmudgett</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/3184/diff/3/?file=53617#file53617line1630" style="color: black; font-weight: bold; text-decoration: underline;">branches/12/main/sorcery.c</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void ast_sorcery_unref(struct ast_sorcery *sorcery)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1483</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ao2_cleanup</span><span class="p">(</span><span class="n">sorcery</span><span class="p">);</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1629</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ao2_cleanup</span><span class="p">(</span><span class="n">sorcery</span><span class="p">);</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1630</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="n">ao2_ref</span><span class="p">(</span><span class="n">sorcery</span><span class="p">,</span> <span class="mi">0</span><span class="p">)</span> <span class="o">==</span> <span class="mi">1</span><span class="p">)</span> <span class="p">{</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1631</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="n">ao2_find</span><span class="p">(</span><span class="n">instances</span><span class="p">,</span> <span class="n">sorcery</span><span class="p">,</span> <span class="n">OBJ_SEARCH_OBJECT</span> <span class="o">|</span> <span class="n">OBJ_UNLINK</span><span class="p">);</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1632</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="p">}</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">There are two red flags here:
1) You are using sorcery after you have released your ref to it.
2) Any time you are looking at the number of references left, there is a design problem. In this case, only the owner of the sorcery object should close/destroy it which will remove it from the instances container. Only the owner of the sorcery object should open/create it which will add it to the instances container. Everyone else will search the instances container for the object to get a reference.</pre>
</blockquote>
<p>On February 6th, 2014, 3:24 p.m. MST, <b>George Joseph</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">There's no concept of weak references so when the sorcery instance is linked to the registry container, the refcount gets bumped without the actual consumer knowing. When the last consumer releases the instance, the instance's refcount is still 1 so the destructor will never run and and remove the entry. Instead of checking the refcount I could simply decrement it automatically after inserting the instance to the container. This way the last consumer to release it will trigger the cleanup.
Now, if you're saying that only 1 consumer should ever call ast_sorcery_open then I need to rethink. The original idea was that multiple consumers could simply call ast_sorcery_open and let it figure out if it needed to create a new one or return the existing one.
</pre>
</blockquote>
<p>On February 6th, 2014, 3:40 p.m. MST, <b>rmudgett</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">You cannot decrement the ref that is held by the container without causing problems for the container when it decrements the the ref on an unlink. You would get a negative ref count.
I was thinking only one creator/owner should ever call the open/destroy.
Bridges and channels have the same kind of problem since they are also put into a global container. They have trigger events to know when they need to be destroyed so they can unlink from the global container.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Yeah, -1 to the auto -1. :)
So I'd have to create a balancing ast_sorcery_close() that did a complete clean of the sorcery instance. Should it clean and destroy (possibly pulling it out from under someone who did a retrieve on it but never -1'd it) or just remove it from the registry and let the last -1 clean it up?
What should happen if the same module (hence the same key) tries to open twice?
BTW, the sorcery instance opened by res_pjsip/config_system gets passed all over pjsip-land, even to other modules, without anyone ever doing a +1 on it. I'd want to go clean those up as well but don't think it should be part of this patch.
</pre>
<br />
<p>- George</p>
<br />
<p>On February 6th, 2014, 1:07 p.m. MST, George Joseph wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By George Joseph.</div>
<p style="color: grey;"><i>Updated Feb. 6, 2014, 1:07 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-22537">ASTERISK-22537</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Create sorcery instance registry as a precursor to creating a generic dialplan function that can retrieve parameters from a sorcery-based config file.
ast_sorcery_init now creates a hashtab as a registry.
ast_sorcery_open now checks the hashtab for an existing sorcery instance matching the caller's module name. If it finds one, it bumps the refcount and returns it. If not, it creates a new sorcery instance, adds it to the hashtab, then returns it.
ast_sorcery_retrieve_by_module_name is a new function that does a hashtab lookup by module name. It can be called by the future dialplan function.
A side effect of this patch is that a module can only have 1 sorcery instance (because it's the key for the hashtab). res_pjsip/config_system needed a small change to share the main res_pjsip sorcery instance.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Made sure that users of sorcery (mostly res_pjsip) continued to load their configs correctly.
Made sure there were no ill effects on res_pjsip from config_system sharing the same sorcery instance as the rest of the pjsip infrastructure.
Made sure that config_system was properly marked as 'not reloadable' and that it was maintaining it's original values when res_pjsip was reloaded.
ernie*CLI> test execute category /main/sorcery/
Running all available tests matching category /main/sorcery/
START /main/sorcery/ - configuration_file_wizard_retrieve_multiple_all
END /main/sorcery/ - configuration_file_wizard_retrieve_multiple_all Time: <1ms Result: PASS
[snip]
START /main/sorcery/ - open
END /main/sorcery/ - open Time: <1ms Result: PASS
START /main/sorcery/ - wizard_registration
END /main/sorcery/ - wizard_registration Time: <1ms Result: PASS
43 Test(s) Executed 43 Passed 0 Failed
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>branches/12/tests/test_sorcery.c <span style="color: grey">(407566)</span></li>
<li>branches/12/res/res_pjsip/include/res_pjsip_private.h <span style="color: grey">(407566)</span></li>
<li>branches/12/res/res_pjsip/config_system.c <span style="color: grey">(407566)</span></li>
<li>branches/12/res/res_pjsip.c <span style="color: grey">(407566)</span></li>
<li>branches/12/main/sorcery.c <span style="color: grey">(407566)</span></li>
<li>branches/12/include/asterisk/sorcery.h <span style="color: grey">(407566)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3184/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>