<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/126105/">https://git.reviewboard.kde.org/r/126105/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 18th, 2015, 8:06 nachm. UTC, <b>Thomas Lübking</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Errr... you intend to crash applications depending on whether some file is present??
Please fix the actual bug instead of such workaround, got a backtrace?</p></pre>
</blockquote>
<p>On November 18th, 2015, 8:22 nachm. UTC, <b>Boudhayan Gupta</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Err... it <strong style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">doesn't</strong> cause a crash with the patch, causes a crash without it. This is a perfectly valid fix - this function is <strong style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">supposed</strong> to return false if the database could not be opened, for any reason whatsoever.</p></pre>
</blockquote>
<p>On November 18th, 2015, 8:25 nachm. UTC, <b>Thomas Lübking</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I didn't say this patch would introduce a crash.
I said that "something" crashes for "some" reason and you work around that by testing whether a file is present.
Your own commit message clearly says that if - for any reason - ~/.local/share/baloo/index exists (eg. i touch it), things will crash again, what means that the actual problem is not resolved but just shadowed.</p></pre>
</blockquote>
<p>On November 18th, 2015, 8:37 nachm. UTC, <b>Boudhayan Gupta</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Why would you want to touch ~/.local/share/baloo/index, if you disable baloo?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Let me explain - balooctl disable works by disabling indexing and removing ~/.local/share/baloo/index. If baloo is disabled, that file is not supposed to exist. A couple of months ago we were even trying to figure out how that file came to exist on systems with Baloo disabled and now I've found out. So this fixes that too.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Now the real problem. Before this patch, Baloo::Database::open() would just create an empty database. Fair enough, except that db->open() would return true in places where you clearly have an invalid database. This would still work (i.e., not crash), because invalid in this context is empty. However, once you select a ton of files (the number is very weird - 158 or something) LMDB would scream at you with this problem - <strong style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">MDB_READERS_FULL: Environment maxreaders limit reached</strong></p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There are two ways to solve this - increase the maxreaders limit in LMDB, or return false if the database doesn't exist. The first option is when you want to go all Linus - <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">we do not break userspace</em>, but this way is the more correct way of solving this, because again, on systems where Baloo is disabled, the index file isn't supposed to exist at all.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I was afraid I was opening up a can of worms by adding yet another condition for db->open() to return false, because I was afraid there were places where a check on db->open()'s return value was missing (and someone would try to do operations on an invalid database). However, I've done all the things that used to trigger crashes before and nothing's crashed yet. With Baloo being both enabled and disabled.</p></pre>
</blockquote>
<p>On November 18th, 2015, 9:14 nachm. UTC, <b>Thomas Lübking</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That sounds as if a ton of jobs is started which all try to access the database?
W/o knowing the details on what is going on, there should probably:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">a) a sane limit per client on how many DB jobs to perform at once (general performance issue)
b) a sanity check on the index file ie. testing whether it contains some significant bytes or at least isn't empty.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I guess (b) should actually done by lmdb, but it won't hurt to do it on baloo as well</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Is this also a problem if the file is actually a valid DB, but baloo just disabled?</p>
<hr style="text-rendering: inherit;margin: 0;padding: 0;white-space: normal;border: 1px solid #ddd;line-height: inherit;" />
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">General remark: I guess valid databases should not be deleted but at best be renamed with de/activation, no matter what else happens with this patch.</p></pre>
</blockquote>
<p>On November 19th, 2015, 6:45 vorm. UTC, <b>Boudhayan Gupta</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'll put this down to a LMDB bug (I suspect some race condition) because when the db is active and indexing is enabled, the crash doesn't happen. Therefore I see no sense in increasing the limit. As for b), then I'd have to know the LMDB file format to do it in Baloo (by default an empty database as created by this method is 8KB in size, so there's some sort of data in it).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For your general remark (rename dbs instead of deleting them) - you'd have to ask Vishesh. That's a design decision left to the maintainer. I'm just a drive-by patcher who's somewhat familiar with the codebase because I've fixed quite a few crashes in other apps when Baloo is disabled ;-)</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Therefore I see no sense in increasing the limit.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Not "increase", the opposite - but Vishesh already argued in the same direction ;-)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It would seem that one of mdb_dbi_open, mdb_dbi_stat or mdb_dbi_flags should better return !0 if the database is invalid ...?</p></pre>
<br />
<p>- Thomas</p>
<br />
<p>On November 19th, 2015, 7:11 vorm. UTC, Boudhayan Gupta 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 Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.</div>
<div>By Boudhayan Gupta.</div>
<p style="color: grey;"><i>Updated Nov. 19, 2015, 7:11 vorm.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
baloo
</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;">Add a check in Baloo::Database::open() to return false if we're opening the database in ReadOnly mode and the database doesn't exist. This fixes a crash in Dolphin when Baloo isn't running.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This isn't the entire fix - one will also have to remove ~/.local/share/baloo/index to not crash anymore; this patch prevents the file from being recreated everytime Baloo::Database::open() is run, and re-causing the crash.</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;">Builds, runs, doesn't crash anymore, the works.</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/engine/database.cpp <span style="color: grey">(4f0579f)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/126105/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>