<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://git.reviewboard.kde.org/r/118458/">https://git.reviewboard.kde.org/r/118458/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Very good to see that it brings the FAM backend to be more in line with the others, behavior-wise.
Just a few small issues.</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/118458/diff/1/?file=277318#file277318line297" style="color: black; font-weight: bold; text-decoration: underline;">autotests/kdirwatch_unittest.cpp</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">297</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">return</span> <span class="n">verifySignalPath</span><span class="p">(</span><span class="n">spyDeleted</span><span class="p">,</span> <span class="s">"deleted(QString)"</span><span class="p">,</span> <span class="n">expectedPath</span><span class="p">)</span> <span class="o">&&</span> <span class="n">verifySignalPath</span><span class="p">(</span><span class="n">spyCreated</span><span class="p">,</span> <span class="s">"deleted(QString)"</span><span class="p">,</span> <span class="n">expectedPath</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">It seems that the second call should pass "created(QString)" rather than "deleted(QString)".</pre>
</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/118458/diff/1/?file=277318#file277318line523" style="color: black; font-weight: bold; text-decoration: underline;">autotests/kdirwatch_unittest.cpp</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; ">void KDirWatch_UnitTest::testDeleteAndRecreateFile() // Useful for /etc/localtime for instance</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">491</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">watch</span><span class="p">.</span><span class="n">internalMethod</span><span class="p">()</span> <span class="o">==</span> <span class="n">KDirWatch</span><span class="o">::</span><span class="n">QFSWatch</span><span class="p">)</span> <span class="p">{</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The commit log doesn't mention any changes related to QFileSystemWatcher, only to the FAM backend. Are you sure you wanted to commit this? If it works then of course I'm happy with it (but maybe in a separate commit then)</pre>
</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/118458/diff/1/?file=277318#file277318line677" style="color: black; font-weight: bold; text-decoration: underline;">autotests/kdirwatch_unittest.cpp</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; ">void KDirWatch_UnitTest::testHardlinkChange()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">645</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="o">::</span><span class="n">link</span><span class="p">(</span><span class="n">QFile</span><span class="o">::</span><span class="n">encodeName</span><span class="p">(</span><span class="n">testFile</span><span class="p">).</span><span class="n">constData</span><span class="p">(),</span> <span class="n">QFile</span><span class="o">::</span><span class="n">encodeName</span><span class="p">(</span><span class="n">existingFile</span><span class="p">).</span><span class="n">constData</span><span class="p">());</span> <span class="c1">// make ExistingFile "point" to TestFile</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">669</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n"><span class="hl">QVERIFY</span></span><span class="p"><span class="hl">(</span></span><span class="o">::</span><span class="n">link</span><span class="p">(</span><span class="n">QFile</span><span class="o">::</span><span class="n">encodeName</span><span class="p">(</span><span class="n">testFile</span><span class="p">).</span><span class="n">constData</span><span class="p">(),</span> <span class="n">QFile</span><span class="o">::</span><span class="n">encodeName</span><span class="p">(</span><span class="n">existingFile</span><span class="p">).</span><span class="n">constData</span><span class="p">())</span><span class="hl"> </span><span class="o"><span class="hl">==</span></span><span class="hl"> </span><span class="mi"><span class="hl">0</span></span><span class="p"><span class="hl">)</span>;</span> <span class="c1">// make ExistingFile "point" to TestFile</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This could actually use QFile::link(), would be more readable.</pre>
</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/118458/diff/1/?file=277319#file277319line636" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/io/kdirwatch.cpp</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; ">bool KDirWatchPrivate::useFAM(Entry *e)</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">636</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">startedEvent</span> <span class="o">=</span> <span class="nb">false</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I'm not 100% sure about the meaning of this boolean variable. (we started an event?)
Maybe it can be called something more explicit (waitForInitialFAMEvents? or something that makes sense to you ;))</pre>
</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/118458/diff/1/?file=277319#file277319line1633" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/io/kdirwatch.cpp</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; ">void KDirWatchPrivate::famEventReceived()</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">1629</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">e</span><span class="o">-></span><span class="n">m_famReportedSeen</span> <span class="o">=</span> <span class="nb">true</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Technically this could be an event about a completely unrelated file to the one we're waiting to see, couldn't it?
</pre>
</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/118458/diff/1/?file=277319#file277319line1638" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/io/kdirwatch.cpp</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; ">void KDirWatchPrivate::famEventReceived()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1609</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">isNoisyFile</span><span class="p">(</span><span class="n">fe</span><span class="o">-></span><span class="n">filename</span><span class="p">))</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">1634</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">isNoisyFile</span><span class="p">(</span><span class="n">fe</span><span class="o">-></span><span class="n">filename</span><span class="p">))</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">For performance reasons it was good for this check to be as early as possible. But of course the risk is an infinite loop if we wait to "see" such a file and we never see it. The solution could be to ensure that we never start watching such a file in the first place...
Anyhow, not a blocker.</pre>
</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/118458/diff/1/?file=277319#file277319line1719" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/io/kdirwatch.cpp</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; ">void KDirWatchPrivate::checkFAMEvent(FAMEvent *fe)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1690</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">// We were waiting for this new file/dir to be created</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1706</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">// We were waiting for this new file/dir to be created<span class="hl">. We don't actual</span></span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">actual -> actually ?</pre>
</div>
<br />
<p>- David Faure</p>
<br />
<p>On June 1st, 2014, 9:03 p.m. UTC, Matthew Dawson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.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 KDE Frameworks.</div>
<div>By Matthew Dawson.</div>
<p style="color: grey;"><i>Updated June 1, 2014, 9:03 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://bugs.kde.org/show_bug.cgi?id=329802">329802</a>
</div>
<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;">Fix KDirWatch to act more reliably against various backends.
- When KDirWatch used a FAM backend, it wouldn't actually wait for the
necessary watches to be put in place before returning, allowing for a race
where an application may think a watch is in place when it really isn't.
This was easily seen using gamin and running the testDeleteAndRecreateFile
test. Fix by delaying useFAM until the watch is in place by waiting until
the EndExist FAM event is received. This adds ~100-200ms per watch with
gamin.
- When a file is deleted and recreated, and scanEntry detects it, the dirty
signal would be emitted. Fix to emit a deleted + created signal, as expected.
- When a deleted signal was requested, other signals were dropped. Due to
the above point, this would stop the created signal from being emitted. Now,
all signals are emitted, even when a delete is detected.
- When watching recreated files, the created signal could get lost as there
was a race between when the created signal would be generated and its signal
spy would be created. Fix by making sure the spy is created before the signal
could be emitted.
- Make sure the created signal isn't emitted twice with both FAM and the
polling timer. This occurs as FAM doesn't fix up the fact the entry has been
created, and the poller thus assumes it needs to notify the world. Fix by
having FAM not emit the event in this case.
BUG: 329802
Note that due to a bug in the FAM daemon (which doesn't exist in the gamin daemon), the testDeleteAndRecreateDir unit test fails as FAM doesn't detect the folder deletion. This test previously passed due to the race condition outlined in bug 329802, which was fixed in this RR. I've confirmed this is a bug in FAM using a minimal program talking directly to the daemon, and watching the strace output from FAM. FAM doesn't detect when a folder it is watching directly is deleted. It does detect when a folder inside a folder it is watching is deleted, however.
I think this should go in as is, as the unit test failing exposes the actual problem, instead of hiding behind a race when it doesn't really work. If running against FAM is important, I can see about tracking down the issue.</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;">Unit tests all pass now with gamin. Using FAM, one test fails as described above, but I've confirmed the daemon itself is causing the issue.</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>autotests/kdirwatch_unittest.cpp <span style="color: grey">(4e638e089012276dc8b17b9d960732a161cc3be1)</span></li>
<li>src/lib/io/kdirwatch.cpp <span style="color: grey">(cc6feec4d2b2598e50c853172bbc295a8c719bc2)</span></li>
<li>src/lib/io/kdirwatch_p.h <span style="color: grey">(cf63c41745091cfedcf1b27c9e4aa77d3db6b31a)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/118458/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>