The code has some preconditions on supplied values when interacting
with the DB. Instead of silently corrupting the DB with bogus values
when using non-debug builds, check any provided arguments.
Details
Details
- Reviewers
ngraham poboiko dhaumann - Group Reviewers
Baloo Frameworks - Commits
- R293:75db0004ad02: Replace several Q_ASSERTs with proper checks
Diff Detail
Diff Detail
- Repository
- R293 Baloo
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
If it was silently corrupting the DB maybe the right solution would have been to integrate baloo properly with KCrash.
Like @sitter did in https://phabricator.kde.org/D15573.
| src/engine/documenturldb.cpp | ||
|---|---|---|
| 44–46 | Would it make sense to include a warning then? If it's a wrong input, we'll need ways to debug it now that it won't be giving a backtrace. | |
| src/engine/documenturldb.cpp | ||
|---|---|---|
| 44–46 | This may be accessed quite frequently, so we would get a lot of warnings, spamming the journal, and slowing down the system. A backtrace is useless here, as the code processes whatever it finds in the database. The interesting part is how the bad data ended up in the db. Doing the checks here avoids spreading any existing corruption. The long term solution is to:
Both are on my TODO list, but this will take some time. | |
| src/engine/documenturldb.cpp | ||
|---|---|---|
| 44–46 | Shouldn't asserts help to find where the library is being misused? | |
| src/engine/documenturldb.cpp | ||
|---|---|---|
| 44–46 | You gain no information if you crash here. Not the code is wrong, but the processed data. Most importantly, when somebody runs this code, compiled without DEBUG, we will run into more and more silent corruption. As soon as baloo is in a state where it is selfhealing, adding logging becomes an option, currently it is not. | |
Comment Actions
Silently - it does not crash, garbage data ends up in the database for (mostly) unknown reasons.
Only known reason for zero IDs are races due to renames and similar, where e.g. a file is added with some path, and the parent does not exist microseconds later (at least not under the previous name).
Comment Actions
I am very much in favour of this change.
In fact, we had this discussion already a long time ago on the mailing list, namely that baloo has (had?) zero error handling and simply assuming that everything works by adding some Q_ASSERTS.
@bruns You are working on this code for quite a time now. I trust you know very much what you are doing much more than possibly anyone else. I know that you would like to get a better review, but having a review open for 5+ months clearly shows we have an issue elsewhere.
Please proceed and commit - I am taking the liberty to give a +2: If there are other issues, then these can still be fixed in followup commits.