<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/1719/">https://reviewboard.asterisk.org/r/1719/</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 8th, 2012, 2:33 a.m., <b>wdoekes</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/1719/diff/1/?file=23900#file23900line160" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/include/asterisk/res_odbc.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">struct odbc_obj *ast_odbc_retrieve_transaction_obj(struct ast_channel *chan, const char *objname);</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">160</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="kt">int</span> <span class="n">ast_odbc_sanity_check_wrlocked</span><span class="p">(</span><span class="k">struct</span> <span class="n">odbc_obj</span> <span class="o">*</span><span class="n">obj</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;">+1 on the _wrlocked suffix. It beats '2' any single day.</pre>
</blockquote>
<p>On February 8th, 2012, 1:48 p.m., <b>Tilghman Lesher</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;">I don't really agree with that. The problem comes to any new developer who sees a host of different APIs and wonders which one is the latest. Adding a '2' makes it very clear that it's a later version of the function. I'd also prefer if the underlying 2-argument function were exposed, instead of hiding it (and thus permitting new users to pass either 1 or 0 themselves), instead of doubling the number of APIs they need to remember.</pre>
</blockquote>
<p>On February 8th, 2012, 2:15 p.m., <b>wdoekes</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;">If you put it like that, sure. But that only makes sense if your second preference is met; exposing the lock_type integer. And in this case, we'd have to know which integer means what (was 0 unlocked or readlocked?), which means adding defines or enums which we have to keep track of.
Having said that, I wonder why the function is exposed at all. No one outside res_odbc is using it, except perhaps 3rd party callers? And I believe you already pointed out that we're already breaking things for them: they won't have the read-lock on it at all.
</pre>
</blockquote>
<p>On February 8th, 2012, 2:57 p.m., <b>Tilghman Lesher</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;">Well, that creates another problem entirely. Currently, the way the code is written, sanity_check has no lock at all, and if we require a lock, then it should acquire it in the old API function. Separately, we should be able to indicate (in the new API) that we already have a read lock, which may need to be escalated to a write lock (with an emitted warning), and finally, we should be able to indicate that we already have a write lock, which does not need to be escalated (and thus avoid an emitted warning).
I considered some of these issues when I wrote the code, which is why I used a mutex instead of a rwlock. Rwlocks are great for concurrency, but they considerably complicate the code when APIs need the ability to escalate locks. (Not to mention that if an API escalates to a write lock and returns to a caller that otherwise kept a persistent read lock, you have none of the advantages of having used rwlocks in the first place).</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;">Actually, there probably isn't any problem with just using ast_odbc_sanity_check even if you have a write lock... at least not compared to the previous behavior. It will just unnecessarily release the write lock so that it can get it again in that case, which is only a problem if you need a guarantee that obj isn't changed during that operation. That guarantee wasn't in place before, so there isn't really any new disadvantage brought about by still using that wrapper and we aren't messing up unknown third party code with these changes (well, in this respect anyway).
It's definitely preferable to use the wrlocked variant though when you have a write lock of course since in that case releasing the lock is just completely unnecessary.
As for exposing the full function, I don't really know how I feel about that. Like Walter said, just because you have to remember one less function doesn't mean you don't have to remember just as much extra junk with the extra input. It also means one more extra function in the API to accomplish the same general task. I think the code is more maintainable if we have leave this as an unexposed helper function because there is less need to decipher the same function with different names and arguments as actually being the same function, and again, the audiohook read_frame function in 10+ is a recent example of this having precedent.</pre>
<br />
<p>- jrose</p>
<br />
<p>On February 8th, 2012, 8:47 a.m., jrose wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers, Mark Michelson, Tilghman Lesher, rmudgett, and wdoekes.</div>
<div>By jrose.</div>
<p style="color: grey;"><i>Updated Feb. 8, 2012, 8:47 a.m.</i></p>
<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;">This is a continuation of wdoekes patch seen in https://reviewboard.asterisk.org/r/1622/
This adds Tilghman Lesher's suggested changes.</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;">I started by confirming that the segfault could be easily reproduced with wdoekes' setup mentioned in the issue report.
After finishing the changes to the patch, I confirmed that following the same steps no longer produced the segfault, undid the patch to double check that I was doing it right, and then decided it was safe to post this review.</pre>
</td>
</tr>
</table>
<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-19011">ASTERISK-19011</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/1.8/res/res_odbc.c <span style="color: grey">(354164)</span></li>
<li>/branches/1.8/include/asterisk/res_odbc.h <span style="color: grey">(354164)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1719/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>