<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/129267/">https://git.reviewboard.kde.org/r/129267/</a>
</td>
</tr>
</table>
<br />
<p>
Fix it, then Ship it!
</p>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm fine with the proposal but would like for the proposed <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">posix_fallocate</code> reimplementation to be improved for portability and resiliency to errors.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If the issues are addressed please commit.</p></pre>
<br />
<div>
<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://git.reviewboard.kde.org/r/129267/diff/1/?file=482987#file482987line91" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/caching/posix_fallocate_mac.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<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">91</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp">#ifdef cplusplus</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This should be <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">__cplusplus</code> (if <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">cplusplus</code> works, it's a non-portable extension).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Better yet, we could probably get rid of this linkage specifier entirely, since this function is used entirely within our C++ source anyways. But I'll leave that up to you to decide.</p></pre>
</div>
</div>
<br />
<div>
<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://git.reviewboard.kde.org/r/129267/diff/1/?file=482987#file482987line97" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/caching/posix_fallocate_mac.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<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">97</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">fstore_t</span> <span class="n">store</span> <span class="o">=</span> <span class="p">{</span><span class="n">F_ALLOCATECONTIG</span><span class="p">,</span> <span class="n">F_PEOFPOSMODE</span><span class="p">,</span> <span class="mi">0</span><span class="p">,</span> <span class="n">offset</span> <span class="o">+</span> <span class="n">len</span><span class="p">};</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Given that the user has some input into the size of the cache I'd prefer to have some kind of integer overflow check on offset + len (since off_t is a signed integer type, this addition is susceptible to undefined behavior). A potential overflow check is available at http://stackoverflow.com/a/1514309, and maybe Qt has something as well.</pre>
</div>
</div>
<br />
<div>
<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://git.reviewboard.kde.org/r/129267/diff/1/?file=482987#file482987line100" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/caching/posix_fallocate_mac.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<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">100</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="o">-</span><span class="mi">1</span> <span class="o">==</span> <span class="n">ret</span><span class="p">)</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'd prefer the error check is consistent with the style used below at line 104, ideally <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">(-1 == ret)</code> or <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">(ret < 0)</code> at both locations.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This will make it clear that both conditionals are intended to check for errors as opposed to other potential uses of the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">fcntl()</code> output.</p></pre>
</div>
</div>
<br />
<div>
<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://git.reviewboard.kde.org/r/129267/diff/1/?file=482987#file482987line108" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/caching/posix_fallocate_mac.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<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">108</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">off_t</span> <span class="n">curPos</span> <span class="o">=</span> <span class="n">lseek</span><span class="p">(</span><span class="n">fd</span><span class="p">,</span> <span class="mi">0</span><span class="p">,</span> <span class="n">SEEK_CUR</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Given that the ftruncate man page is documented as leaving the current file offset unchanged, why are we trying to read the old offset and reset it later? Does ftruncate fail on Mac OS X if the offset is set wrong?
The reason I ask is that we don't check the return value of lseek() here and while it seems like it should be safe it's at least theoretically possible to have it return an error.</pre>
</div>
</div>
<br />
<div>
<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://git.reviewboard.kde.org/r/129267/diff/1/?file=482987#file482987line115" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/caching/posix_fallocate_mac.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<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">115</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp">#ifdef cplusplus</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Likewise, needs to be <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">__cplusplus</code>.</p></pre>
</div>
</div>
<br />
<p>- Michael Pyne</p>
<br />
<p>On October 26th, 2016, 6:23 p.m. UTC, René J.V. Bertin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Software on Mac OS X and KDE Frameworks.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Oct. 26, 2016, 6:23 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kcoreaddons
</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Mac OS X doesn't have <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">posix_fallocate()</code> but as written in the comments, this function can be emulated (at least for simple use with <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">offset=0</code>).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch introduces such an emulation, based on <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">mozilla::fallocation()</code>. There's so little of that function left that it may be overkill to maintain its original copyright statement but I'll leave that to the legalese experts.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Works as intended on OS X 10.9.5 as far as I can tell; original file contents were not overwritten in my testing.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The autotest succeeds (with an additional debug statement left out of the patch):</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span></span><span style="color: #666666">></span> <span style="color: #008000; font-weight: bold">kf5-kcoreaddons</span><span style="color: #666666">/</span><span style="color: #008000; font-weight: bold">work</span><span style="color: #666666">/</span><span style="color: #008000; font-weight: bold">build</span><span style="color: #666666">/</span><span style="color: #008000; font-weight: bold">autotests</span><span style="color: #666666">/</span><span style="color: #008000; font-weight: bold">kshareddatacachetest</span>
<span style="color: #666666">*********</span> <span style="color: #008000; font-weight: bold">Start</span> <span style="color: #008000; font-weight: bold">testing</span> <span style="color: #008000; font-weight: bold">of</span> <span style="color: #008000; font-weight: bold">KSharedDataCacheTest</span> <span style="color: #666666">*********</span>
<span style="color: #008000; font-weight: bold">Config</span><span style="color: #666666">:</span> <span style="color: #008000; font-weight: bold">Using</span> <span style="color: #008000; font-weight: bold">QtTest</span> <span style="color: #008000; font-weight: bold">library</span> <span style="color: #008000; font-weight: bold">5</span><span style="color: #0000FF; font-weight: bold">.6.1</span><span style="color: #666666">,</span> <span style="color: #008000; font-weight: bold">Qt</span> <span style="color: #008000; font-weight: bold">5</span><span style="color: #0000FF; font-weight: bold">.6.1</span> <span style="color: #666666">(</span><span style="color: #008000; font-weight: bold">x86_64-little_endian-lp64</span> <span style="color: #008000; font-weight: bold">shared</span> <span style="color: #666666">(</span><span style="color: #008000; font-weight: bold">dynamic</span><span style="color: #666666">)</span> <span style="color: #008000; font-weight: bold">release</span> <span style="color: #008000; font-weight: bold">build</span><span style="color: #666666">;</span> <span style="color: #008000; font-weight: bold">by</span> <span style="color: #008000; font-weight: bold">Clang</span> <span style="color: #008000; font-weight: bold">6</span><span style="color: #0000FF; font-weight: bold">.0</span> <span style="color: #666666">(</span><span style="color: #008000; font-weight: bold">clang-600</span><span style="color: #0000FF; font-weight: bold">.0.57</span><span style="color: #666666">)</span> <span style="color: #666666">(</span><span style="color: #008000; font-weight: bold">Apple</span><span style="color: #666666">))</span>
<span style="color: #008000; font-weight: bold">PASS</span> <span style="color: #666666">:</span> <span style="color: #008000; font-weight: bold">KSharedDataCacheTest</span><span style="color: #666666">:</span><span style="color: #AA22FF">:initTestCase</span><span style="color: #666666">()</span>
<span style="color: #008000; font-weight: bold">QWARN</span> <span style="color: #666666">:</span> <span style="color: #008000; font-weight: bold">KSharedDataCacheTest</span><span style="color: #666666">:</span><span style="color: #AA22FF">:simpleInsert</span><span style="color: #666666">()</span> <span style="color: #008000; font-weight: bold">void</span> <span style="color: #008000; font-weight: bold">KSharedDataCache</span><span style="color: #666666">:</span><span style="color: #AA22FF">:Private</span><span style="color: #666666">:</span><span style="color: #AA22FF">:mapSharedMemory</span><span style="color: #666666">()</span> <span style="color: #008000; font-weight: bold">posix_fallocated</span> <span style="color: #008000; font-weight: bold">5273696</span> <span style="color: #008000; font-weight: bold">bytes</span> <span style="color: #008000; font-weight: bold">for</span> <span style="color: #008000; font-weight: bold">file</span> <span style="color: #BA2121">"/Users/bertin/.cache/myTestCache.kcache"</span>
<span style="color: #008000; font-weight: bold">PASS</span> <span style="color: #666666">:</span> <span style="color: #008000; font-weight: bold">KSharedDataCacheTest</span><span style="color: #666666">:</span><span style="color: #AA22FF">:simpleInsert</span><span style="color: #666666">()</span>
<span style="color: #008000; font-weight: bold">PASS</span> <span style="color: #666666">:</span> <span style="color: #008000; font-weight: bold">KSharedDataCacheTest</span><span style="color: #666666">:</span><span style="color: #AA22FF">:cleanupTestCase</span><span style="color: #666666">()</span>
<span style="color: #008000; font-weight: bold">Totals</span><span style="color: #666666">:</span> <span style="color: #008000; font-weight: bold">3</span> <span style="color: #008000; font-weight: bold">passed</span><span style="color: #666666">,</span> <span style="color: #008000; font-weight: bold">0</span> <span style="color: #008000; font-weight: bold">failed</span><span style="color: #666666">,</span> <span style="color: #008000; font-weight: bold">0</span> <span style="color: #008000; font-weight: bold">skipped</span><span style="color: #666666">,</span> <span style="color: #008000; font-weight: bold">0</span> <span style="color: #008000; font-weight: bold">blacklisted</span>
<span style="color: #666666">*********</span> <span style="color: #008000; font-weight: bold">Finished</span> <span style="color: #008000; font-weight: bold">testing</span> <span style="color: #008000; font-weight: bold">of</span> <span style="color: #008000; font-weight: bold">KSharedDataCacheTest</span> <span style="color: #666666">*********</span>
</pre></div>
</p></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>src/lib/caching/kshareddatacache_p.h <span style="color: grey">(9fd0bb1)</span></li>
<li>src/lib/caching/posix_fallocate_mac.h <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/129267/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/10/26/e0af195f-b484-4647-b211-cc78b5b0584b__posix_fallocate.c">a simple commandline test app</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>