<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/130084/">https://git.reviewboard.kde.org/r/130084/</a>
</td>
</tr>
</table>
<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/130084/diff/2/?file=494464#file494464line89" style="color: black; font-weight: bold; text-decoration: underline;">src/ioslaves/file/file_unix.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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">89</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nf">file_is_on_removable_media</span><span class="p">(</span><span class="kt">int</span> <span class="n">fd</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;">remove newline after "bool" and before function name
(Qt/KF5 coding style)</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/130084/diff/2/?file=494464#file494464line103" style="color: black; font-weight: bold; text-decoration: underline;">src/ioslaves/file/file_unix.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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">103</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">hasMajorMinor</span> <span class="o">&=</span> <span class="n">Solid</span><span class="o">::</span><span class="n">Predicate</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;">&= doesn't do what you think, it's a bit-flag operation and not a boolean operation.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">1=true
2=true
1&2=false</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You need to use = hasMajorMinor &&, for lack of a &&= in the standard.</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/130084/diff/2/?file=494464#file494464line109" style="color: black; font-weight: bold; text-decoration: underline;">src/ioslaves/file/file_unix.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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">109</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">auto</span> <span class="n">blockdev_list</span> <span class="o">=</span> <span class="n">Solid</span><span class="o">::</span><span class="n">Device</span><span class="o">::</span><span class="n">listFromQuery</span><span class="p">(</span><span class="n">hasMajorMinor</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;">const auto...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">so that first() below doesn't detach.</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/130084/diff/2/?file=494464#file494464line229" style="color: black; font-weight: bold; text-decoration: underline;">src/ioslaves/file/file_unix.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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">229</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">bool</span> <span class="n">should_fsync</span> <span class="o">=</span> <span class="n">file_is_on_removable_media</span><span class="p">(</span><span class="n">dest_file</span><span class="p">.</span><span class="n">handle</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;">const bool .. to make it clear it's not modified further down</p></pre>
</div>
</div>
<br />
<p>- David Faure</p>
<br />
<p>On April 17th, 2017, 11:27 a.m. UTC, KJ Tsanaktsidis 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 Frameworks, Oswald Buddenhagen and Thiago Macieira.</div>
<div>By KJ Tsanaktsidis.</div>
<p style="color: grey;"><i>Updated April 17, 2017, 11:27 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kio
</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;">When copying a large-ish file (~1-2GB) from very fast storage to very slow storage (e.g. an NVME SSD to a cheap USB flash drive) on a machine with lots of RAM, Dolphin displays a progress bar which finishes in a fraction of a second (i.e. as fast as it takes to read the source file into the Linux page cache). Unmounting the drive then of course takes a long time, with only an indeterminate spinner.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch adds an option to force fsync during copy jobs, so that the copy progress bar measures how long it will take to actually copy the file to the destination.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I've added two flags - Fsync and FsyncCrossFilesystem - to the JobEnum flag. The former will cause all copy operations to fsync during the copy loop, whilst the latter will only fsync copies that are across different filesystems.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If this patch gets OK'd, I have another patch which adds support for this into the appropriate places in Dolphin. I would think that at least FsyncCrossFilesystem should be the default, but Fsync always might be a little heavy handed. At the least fsync'ing cross-filesystem copies ensures that the unmount won't take forever.</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;">Tested the patch with KDE/Dolphin on Arch Linux, which is version 5.32.0. The diff applies cleanly to master so I assume there shouldn't be any issues there, but I've not actually checked that. As advertised, copying a file to USB flash storage now displays an accurate progress bar.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I experimented with how often fsync should be called on my hardware, and I found calling it every ~1M copied caused no decrease in copy performance whilst still providing accurate progress info. That is the setting I've gone with in this patch. I'm open to suggestions on how this could be tuned better though.</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/ioslaves/file/CMakeLists.txt <span style="color: grey">(b9132ced9d4a08b2cf9f9bbbaa3bd43f026c6469)</span></li>
<li>src/ioslaves/file/ConfigureChecks.cmake <span style="color: grey">(5a83d1b9fbe90c851c774e3b467468d93b5a2bd4)</span></li>
<li>src/ioslaves/file/config-kioslave-file.h.cmake <span style="color: grey">(372f79d01ad4597aae0b2ae62627648fe7680b64)</span></li>
<li>src/ioslaves/file/file_unix.cpp <span style="color: grey">(3c1b9927e3dd2d0134f77caec6e6b24a0356d26f)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/130084/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>