D18664: Baloo engine: treat every non-success code as a failure

This revision was automatically updated to reflect the committed changes. Closed by commit R293:eb68430ae5f4: Baloo engine: treat every non-success code as a failure (authored by valeriymalov). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D18664?vs=53608&id=53838#toc REPOSITORY R293 B

D18664: Baloo engine: treat every non-success code as a failure

bruns accepted this revision. This revision is now accepted and ready to land. REPOSITORY R293 Baloo BRANCH master REVISION DETAIL https://phabricator.kde.org/D18664 To: valeriymalov, #baloo, bruns, poboiko Cc: cullmann, ngraham, bruns, kde-frameworks-devel, #baloo, gennad, domson, ashap

D18664: Baloo engine: treat every non-success code as a failure

valeriymalov updated this revision to Diff 53608. valeriymalov marked 3 inline comments as done. valeriymalov added a comment. - do not ignore results of PostingDB::iter in case of an error/end of db REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18664?vs=522

D18664: Baloo engine: treat every non-success code as a failure

bruns added a comment. @valeriymalov - are you going to update this? REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D18664 To: valeriymalov, #baloo, bruns, poboiko Cc: cullmann, ngraham, bruns, kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, ast

D18664: Baloo engine: treat every non-success code as a failure

bruns requested changes to this revision. bruns added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > poboiko wrote in postingdb.cpp:238 > Can it happen that `rc == MDB_NOTFOUND` just after some iterations of > `MDB_NEXT` operation (i.e. we've reached the end of

D18664: Baloo engine: treat every non-success code as a failure

poboiko added a comment. I've looked through the patch (quite large indeed), apart from the single note I think it's good to go. INLINE COMMENTS > postingdb.cpp:238 > mdb_cursor_close(cursor); > -if (termIterators.isEmpty()) { > +if (rc || termIterators.isEmpty()) { > r

D18664: Baloo engine: treat every non-success code as a failure

bruns added a comment. I think this looks good now, but I would prefer an OK from another developer, as this has become quite large. Readability good profit from `if (rc)` -> `if (rc != MDB_SUCCESS)` resp. `if (rc == 0)` -> `if (rc == MDB_SUCCESS)`, but I am not sure about this. REPOSIT

D18664: Baloo engine: treat every non-success code as a failure

valeriymalov updated this revision to Diff 52229. valeriymalov added a comment. - fix MTimeDB::get loop REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18664?vs=52216&id=52229 BRANCH master REVISION DETAIL https://phabricator.kde.org/D18664 AFFECTED FIL

D18664: Baloo engine: treat every non-success code as a failure

bruns requested changes to this revision. bruns added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > bruns wrote in mtimedb.cpp:110 > `if (rc != MDB_NOTFOUND)` for the message **for the message** - you now only retrieve the first entry, spew an error message,

D18664: Baloo engine: treat every non-success code as a failure

valeriymalov updated this revision to Diff 52216. valeriymalov marked 4 inline comments as done. valeriymalov added a comment. - review warning fixes REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18664?vs=51284&id=52216 BRANCH master REVISION DETAIL ht

D18664: Baloo engine: treat every non-success code as a failure

valeriymalov edited the summary of this revision. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D18664 To: valeriymalov, #baloo, bruns, poboiko Cc: cullmann, ngraham, bruns, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, abrahams

D18664: Baloo engine: treat every non-success code as a failure

bruns added a comment. Also, remove the second paragraph from the summary - a commit message is not a TODO list. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D18664 To: valeriymalov, #baloo, bruns, poboiko Cc: cullmann, ngraham, bruns, kde-frameworks-devel, #baloo,

D18664: Baloo engine: treat every non-success code as a failure

bruns added a comment. Also, these should only be CCBUG, as it is only an assumption it fixes any of these. Actually, I have seen the database return corrupt data, not caused by a failed call, but because the entry did contain garbage. REPOSITORY R293 Baloo REVISION DETAIL https://

D18664: Baloo engine: treat every non-success code as a failure

bruns added a comment. looks almost good to me, can someone else please crosscheck? INLINE COMMENTS > documenturldb.h:140 > +qWarning() << "DocumentUrlDB::del" > + << "This folder still has > sub-files in its cache. It cannot b

D18664: Baloo engine: treat every non-success code as a failure

valeriymalov updated this revision to Diff 51284. valeriymalov marked 4 inline comments as done. valeriymalov added a comment. - fix ::del error logging criteria, don't return without closing cursor in REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18664?vs=5

D18664: Baloo engine: treat every non-success code as a failure

bruns added inline comments. INLINE COMMENTS > valeriymalov wrote in documentdatadb.cpp:107 > Trying to delete a non-existent entry seems like an error to me Deleting is an idempotent operation, so e.g. doing it twice is fine. There may be multiple events queued leading to the deletion of a dat

D18664: Baloo engine: treat every non-success code as a failure

valeriymalov added inline comments. INLINE COMMENTS > bruns wrote in documentdatadb.cpp:107 > see get Trying to delete a non-existent entry seems like an error to me REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D18664 To: valeriymalov, #baloo, bruns, poboiko Cc: cullm

D18664: Baloo engine: treat every non-success code as a failure

valeriymalov updated this revision to Diff 51282. valeriymalov marked 10 inline comments as done. valeriymalov added a comment. - update logging per review REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18664?vs=51252&id=51282 BRANCH master REVISION DETAI

D18664: Baloo engine: treat every non-success code as a failure

bruns added inline comments. INLINE COMMENTS > documentdatadb.cpp:42 > +if (rc) { > +qCDebug(ENGINE) << "DocumentDataDB::create" << mdb_strerror(rc); > +return 0; Warning > documentdatadb.cpp:54 > +if (rc) { > +qCDebug(ENGINE) << "DocumentDataDB::open" << mdb_str

D18664: Baloo engine: treat every non-success code as a failure

valeriymalov added a dependency: D18873: add baloo engine debugging category. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D18664 To: valeriymalov, #baloo, bruns, poboiko Cc: cullmann, ngraham, bruns, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spo

D18664: Baloo engine: treat every non-success code as a failure

valeriymalov updated this revision to Diff 51252. valeriymalov added a comment. - move new logging category to D18873: add baloo engine debugging category REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18664?vs=51251&id=5

D18664: Baloo engine: treat every non-success code as a failure

valeriymalov updated this revision to Diff 51251. valeriymalov marked 3 inline comments as done. valeriymalov added a comment. - clean up rest of the asserts REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18664?vs=50708&id=51251 BRANCH master REVISION DET

D18664: Baloo engine: treat every non-success code as a failure

cullmann added a comment. I think this is a step into the right direction. Thought I am still not sure if we not should go away from a own storage db for all this and port that over to e.g. use the tracker stuff. Given that fixing the issues will be more or less a complete rewrite, too.

D18664: Baloo engine: treat every non-success code as a failure

bruns added a comment. In general, these changes seem worthwhile. But as already said, splitting these into two parts - QDebug etc vs. Error handling - is a must. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D18664 To: valeriymalov, #baloo, bruns, poboiko Cc: ngra

D18664: Baloo engine: treat every non-success code as a failure

valeriymalov planned changes to this revision. valeriymalov added a comment. In D18664#404173 , @poboiko wrote: > 1. Are we actually sure this is gonna fix all those crashes? Otherwise I would suggest using CCBUG instead of BUG inside the commit

D18664: Baloo engine: treat every non-success code as a failure

poboiko added a comment. Nice! I like it, it's definitely much better than `Q_ASSERT_X` macros that are just silently ignored in non-debug builds. Have some nitpicks though: 1. Are we actually sure this is gonna fix all those crashes? Otherwise I would suggest using CCBUG instead of

D18664: Baloo engine: treat every non-success code as a failure

ngraham added subscribers: bruns, ngraham. ngraham added reviewers: bruns, poboiko. ngraham added a comment. Conceptually this seems like it's definitely better than nothing or the status quo. Only checking for `MDB_NOTFOUND` and ignoring other error statuses seems like really bad code hygien

D18664: Baloo engine: treat every non-success code as a failure

valeriymalov updated this revision to Diff 50708. valeriymalov added a comment. - unbreak documentdatadb::contains, oops :( REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18664?vs=50706&id=50708 BRANCH master REVISION DETAIL https://phabricator.kde.org/

D18664: Baloo engine: treat every non-success code as a failure

valeriymalov added a reviewer: Baloo. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D18664 To: valeriymalov, #baloo Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D18664: Baloo engine: treat every non-success code as a failure

valeriymalov created this revision. Herald added projects: Frameworks, Baloo. Herald added subscribers: Baloo, kde-frameworks-devel. valeriymalov requested review of this revision. REVISION SUMMARY Treating only MDB_NOTFOUND as an error leads to use of uninitliazed pointers and handle IDs in o