> On Nov. 18, 2015, 8:06 nachm., Thomas Lübking wrote: > > 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? > > Boudhayan Gupta wrote: > Err... it **doesn't** cause a crash with the patch, causes a crash > without it. This is a perfectly valid fix - this function is **supposed** to > return false if the database could not be opened, for any reason whatsoever. > > Thomas Lübking wrote: > 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. > > Boudhayan Gupta wrote: > Why would you want to touch ~/.local/share/baloo/index, if you disable > baloo? > > 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. > > 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 - **MDB_READERS_FULL: > Environment maxreaders limit reached** > > 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 - *we do not break userspace*, 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. > > 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. > > Thomas Lübking wrote: > 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: > > 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. > > I guess (b) should actually done by lmdb, but it won't hurt to do it on > baloo as well > > Is this also a problem if the file is actually a valid DB, but baloo just > disabled? > > > --- > 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. > > Boudhayan Gupta wrote: > 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). > > 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 ;-)
> Therefore I see no sense in increasing the limit. Not "increase", the opposite - but Vishesh already argued in the same direction ;-) 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 ...? - Thomas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126105/#review88544 ----------------------------------------------------------- On Nov. 19, 2015, 7:11 vorm., Boudhayan Gupta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126105/ > ----------------------------------------------------------- > > (Updated Nov. 19, 2015, 7:11 vorm.) > > > Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa. > > > Repository: baloo > > > Description > ------- > > 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. > > 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. > > > Diffs > ----- > > src/engine/database.cpp 4f0579f > > Diff: https://git.reviewboard.kde.org/r/126105/diff/ > > > Testing > ------- > > Builds, runs, doesn't crash anymore, the works. > > > Thanks, > > Boudhayan Gupta > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
