Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Thomas Lübking]

> 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/
> -

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Boudhayan Gupta]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126105/
---

(Updated Nov. 19, 2015, 7:11 a.m.)


Status
--

This change has been marked as submitted.


Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.


Changes
---

Submitted with commit 1e880e29883561c8330cb81c03b48716ea68616c by Boudhayan 
Gupta to branch master.


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

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Boudhayan Gupta]

> On Nov. 19, 2015, 1:36 a.m., 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.

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 ;-)


- Boudhayan


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126105/#review88544
---


On Nov. 19, 2015, 1:10 a.m., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 19, 2015, 1:10 a.m.)
> 
> 
> 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 
> datab

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Boudhayan Gupta]

> On Nov. 19, 2015, 10:35 a.m., Sune Vuorela wrote:
> > without actualy knowing the code in question, a brief look over gives me 
> > the impression that the error handling could be improved and that would 
> > make this patch not needed.

The assumption here is that if (rc != 0) in normal operation the errors are 
serious enough to warrant crashing the process, not handling it gracefully. 
Granted, I've just discovered one case where this isn't true, but that's when 
indexing is disabled and the db file shouldn't exist, so I've added a special 
check for that.

Also, that patch prevents an empty index from being created even when indexing 
is disabled, which is correct behaviour.


- Boudhayan


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126105/#review88556
---


On Nov. 19, 2015, 1:10 a.m., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 19, 2015, 1:10 a.m.)
> 
> 
> 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

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Boudhayan Gupta]

> On Nov. 19, 2015, 6:03 a.m., Vishesh Handa wrote:
> > I'm a little hesitant about this, but ship it.
> > 
> > A couple of things though -
> > 1. Dolphin should probably not be sending Baloo so many requests if Baloo 
> > is disabled. Perhaps we should fix this in Dolphin.
> > 2. This patch is mostly only for the Baloo::File class which causes the db 
> > to be opened and closed for each operation. It's expensive, but the main 
> > use case that I focussed on was searching. Over there the db is only opened 
> > once per application.

I concur, the errant calls are being made by Baloo-Widgets and we should 
probably fix Baloo-Widgets to not go through the hassle of invoking lower-level 
baloo functions if it's disabled. I'll look into it - is there a simple API in 
Baloo which I can check to see if indexing is disabled? Can I use 
Baloo::IndexerConfig::fileIndexingEnabled() from Baloo-Widgets to do this check?


- Boudhayan


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126105/#review88554
---


On Nov. 19, 2015, 1:10 a.m., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 19, 2015, 1:10 a.m.)
> 
> 
> 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

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Sune Vuorela]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126105/#review88556
---


without actualy knowing the code in question, a brief look over gives me the 
impression that the error handling could be improved and that would make this 
patch not needed.


src/engine/database.cpp (line 104)


SHouldn't there be a if(rc == 0) return false here ?



src/engine/database.cpp (line 137)


if(rc == 0) return false



src/engine/database.cpp (line 167)


if(rc == 0) return false


- Sune Vuorela


On Nov. 18, 2015, 7:40 p.m., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 18, 2015, 7:40 p.m.)
> 
> 
> 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

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Vishesh Handa]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126105/#review88554
---

Ship it!


I'm a little hesitant about this, but ship it.

A couple of things though -
1. Dolphin should probably not be sending Baloo so many requests if Baloo is 
disabled. Perhaps we should fix this in Dolphin.
2. This patch is mostly only for the Baloo::File class which causes the db to 
be opened and closed for each operation. It's expensive, but the main use case 
that I focussed on was searching. Over there the db is only opened once per 
application.

- Vishesh Handa


On Nov. 18, 2015, 7:40 p.m., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 18, 2015, 7:40 p.m.)
> 
> 
> 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

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Thomas Lübking]

> 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.

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.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126105/#review88544
---


On Nov. 18, 2015, 7:40 nachm., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 18, 2015, 7:40 nachm.)
> 
> 
> 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

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Boudhayan Gupta]

> On Nov. 19, 2015, 1:36 a.m., 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.

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.


- Boudhayan


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126105/#review88544
---


On Nov. 19, 2015, 1:10 a.m., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 19, 2015, 1:10 a.m.)
> 
> 
> 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

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Thomas Lübking]

> 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.

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.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126105/#review88544
---


On Nov. 18, 2015, 7:40 nachm., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 18, 2015, 7:40 nachm.)
> 
> 
> 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

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Boudhayan Gupta]

> On Nov. 19, 2015, 1:36 a.m., 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?

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.


- Boudhayan


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126105/#review88544
---


On Nov. 19, 2015, 1:10 a.m., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 19, 2015, 1:10 a.m.)
> 
> 
> 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

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126105/#review88544
---


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?

- Thomas Lübking


On Nov. 18, 2015, 7:40 nachm., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 18, 2015, 7:40 nachm.)
> 
> 
> 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