Re: C++11 & friends

[Martin Graesslin]

On Sunday, September 11, 2016 3:21:21 AM CEST Dominik Haumann wrote:
> Hi all,
> 
> I just saw a commit by Volker turning nullptr into Q_NULLPTR with the
> comment that Visual Studio 2012 does not support nullptr.
> 
> While this change is trivial for obvious reasons, do we really need to do
> that?
> 
> I am raising this question especially since KTextEditor seems to use
> 'nullptr' in several locations now for several releases - and noone
> complained.

I have said so in the past and I will continue to say this: as long as we 
don't have a CI system which can verify the C++11 subset we are allowed to 
use, it's pointless to try to restrict the usage. We are humans and no 
compilers. Nobody can expect that we remember which calls are allowed and 
which not if we have C++14 compliant compilers.

To me the only relevant and allowed subset of C++ is the one which compiles on 
build.kde.org.

Cheers
Martin

signature.asc
Description: This is a digitally signed message part.

Re: C++11 & friends

[Kevin Funk]

On Sunday, 11 September 2016 03:21:21 CEST Dominik Haumann wrote:
> Hi all,
> 
> I just saw a commit by Volker turning nullptr into Q_NULLPTR with the
> comment that Visual Studio 2012 does not support nullptr.
> 
> While this change is trivial for obvious reasons, do we really need to do
> that?

I don't think so.

nullptr is actually supported since VS2010 [1].

Some other remark: I think noone's using anything below VS2015 for testing KDE 
on Windows anyway. I'm expecting compilation to be broken on earlier versions 
of VS.

Cheers,
Kevin

[1] http://en.cppreference.com/w/cpp/compiler_support for evidence
 
> I am raising this question especially since KTextEditor seems to use
> 'nullptr' in several locations now for several releases - and noone
> complained.
> 
> Are we supposed to turn nullptr in KTextEditor also into nullptr, or
> can we take the liberty and ditch Q_NULLPTR completely for all
> frameworks?
> 
> Same also applies to 'override'.
> 
> Best,
> Dominik


-- 
Kevin Funk | kf...@kde.org | http://kfunk.org

signature.asc
Description: This is a digitally signed message part.

Re: Taking over maintainership of Baloo

[Christoph Cullmann]

Hi,

> Hey Boudhayan,
> 
> It's nice that you are stepping up for maintaining baloo. I have no
> objections to that and will try to help you out whenever needed :)
I triaged the bugs a lot today, we are now down to < 70:

https://bugs.kde.org/buglist.cgi?bug_severity=critical&bug_severity=grave&bug_severity=major&bug_severity=crash&bug_severity=normal&bug_severity=minor&bug_status=UNCONFIRMED&bug_status=CONFIRMED&bug_status=ASSIGNED&bug_status=REOPENED&list_id=1385670&product=frameworks-baloo

It would be really great, if you could take a look at some and perhaps
help with adding the needed error checks.

Greetings
Christoph

-- 
- Dr.-Ing. Christoph Cullmann -
AbsInt Angewandte Informatik GmbH  Email: cullm...@absint.com
Science Park 1 Tel:   +49-681-38360-22
66123 Saarbrücken  Fax:   +49-681-38360-20
GERMANYWWW:   http://www.AbsInt.com

Geschäftsführung: Dr.-Ing. Christian Ferdinand
Eingetragen im Handelsregister des Amtsgerichts Saarbrücken, HRB 11234

Re: Taking over maintainership of Baloo

[Pinak Ahuja]

Hey Boudhayan,

It's nice that you are stepping up for maintaining baloo. I have no
objections to that and will try to help you out whenever needed :)

Cheers,
Pinak

--
Thanks
Pinak Ahuja

On 11 September 2016 at 19:40, Boudhayan Gupta  wrote:

> Hi,
>
> On 11 September 2016 at 22:59, Christoph Cullmann 
> wrote:
> > I think the main issue is: There is no need to reproduce them, it is
> clear, that baloo will crash
> > on any problem with lmdb, as "no" errors are handled (or lets say 99% of
> the errors are not handled).
> >
> > The code is full of Q_ASSERT, but no handling of the return codes beside
> that.
>
> Oh, it gets worse. Q_ASSERTs compile to nothing in release mode so you
> don't even have meaningful crashes on those asserts, things just
> mysteriously fail elsewhere. I spent a whole bunch of time coding in
> actual checks after those asserts because Dolphin would crash when
> selecting multiple files with Baloo disabled, and other such magical
> bugs.
>
> > Actually, I think, in most cases the code shall just catch the error
> cases and do nothing (or purge data
> > until all is fine again) but not like now: crash or assert.
> >
> > That is not acceptable for something that runs per default ;=)
> >
> > Just looking at the code, you see things like:
> >
> > 1) Baloo::Database: needs a mutex, as many threads might call ::open
> (e.g. in krunner) => corruption, dead
> >
> > 2) inproper cleanup: I doubt one can  mdb_txn_abort(txn); if already
> mdb_txn_begin(...) failed
> >
> > 3) needs everywhere return code checks, e.g. in PostingList
> PostingDB::get(const QByteArray& term), we have
> > tons of bugs about random crashs afterwards:
> https://bugs.kde.org/show_bug.cgi?id=367480
> >
> > 4) 32-bit systems supported at all? ATM, after 1GB of indexing, that was
> it, no more baloo or any other application
> > calling any of the accessors of e.g. Query (if you have bad luck).
>
> If you have the time to fix those things you've listed above, awesome
> - I'd love to look at the RRs. Lessens my workload.
>
> Otherwise I'll look at this on the weekends when I have no sysadmin
> things to do.
>
> >
> >>
> >>> Beside that, could we get some baloo-b...@kde.org list as default
> assignee
> >>> for all baloo bugs instead of "one" person?
> >>
> >> If we're going to do it this way, why not just make it the default
> >> frameworks mailing list, or the frameworks bugs list if there is one?
> > I am not sure, if that will make people happy ;=)
>
> Let's get other people involved, too ;-)
>
> Thanks,
> Boudhayan
>

Re: Taking over maintainership of Baloo

[Martin Steigerwald]

Am Sonntag, 11. September 2016, 20:07:36 CEST schrieb Dominik Haumann:
> indeed I also did take a look at the bug list, and out of the 200
> bugs, without much exaggeration almost all of them are crashes.
> 
> In fact, I believe it's important to CC the plasma-devel mailing list
> here: Given we are about to release a LTS Plasma 5.8, and baloo is
> tightly integrated into Plasma, Plasma will still contain lots and
> lots of crashes.
> 
> Maybe the Plasma people are also interested in taking e.g. 2-3 days to
> look into this.

As a crashing Baloo can also take Kickoff menu or KRunner with it I strongly 
agree with this. That said I didn´t see a crash in a longer time.

-- 
Martin

Re: Review Request 128892: Open baloo lmdb database read-only beside in baloo_file/baloo_file_extractor + balooctl (for some commands) + unit tests

[Christoph Cullmann]

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

(Updated Sept. 12, 2016, 12:39 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, Plasma, Boudhayan Gupta, and Vishesh Handa.


Changes
---

Submitted with commit 02047b524a176da447d8c96e15c7e2abae8339ae by Christoph 
Cullmann to branch master.


Repository: baloo


Description
---

At the moment, any application that uses baloo can corrupt the db.
Now, only the things that need to write to it open it with read-write.
This only works as long as the library exposes only read-only things like 
Query/...


Diffs
-

  src/engine/database.h 6ccb2a5 
  src/engine/database.cpp 6a433c7 
  src/file/extractor/app.cpp 0ca7276 
  src/lib/file.cpp cbbc912 
  src/lib/searchstore.cpp 060a4fd 
  src/lib/taglistjob.cpp 76ac8ff 
  src/qml/experimental/monitor.cpp 11c06ae 
  src/tools/balooctl/main.cpp 2a6b175 
  src/tools/balooctl/statuscommand.cpp 1a56c64 
  src/tools/balooshow/main.cpp f45f2e0 

Diff: https://git.reviewboard.kde.org/r/128892/diff/


Testing
---

Unit tests still work, balooctl seems to do something.


Thanks,

Christoph Cullmann

Re: Review Request 128892: Open baloo lmdb database read-only beside in baloo_file/baloo_file_extractor + balooctl (for some commands) + unit tests

[Christoph Cullmann]

> On Sept. 11, 2016, 9:35 p.m., Vishesh Handa wrote:
> > This is awesome. Ship it.

Lets hope it is awesome correct.


- Christoph


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


On Sept. 11, 2016, 7:19 p.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128892/
> ---
> 
> (Updated Sept. 11, 2016, 7:19 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma, Boudhayan Gupta, and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> At the moment, any application that uses baloo can corrupt the db.
> Now, only the things that need to write to it open it with read-write.
> This only works as long as the library exposes only read-only things like 
> Query/...
> 
> 
> Diffs
> -
> 
>   src/engine/database.h 6ccb2a5 
>   src/engine/database.cpp 6a433c7 
>   src/file/extractor/app.cpp 0ca7276 
>   src/lib/file.cpp cbbc912 
>   src/lib/searchstore.cpp 060a4fd 
>   src/lib/taglistjob.cpp 76ac8ff 
>   src/qml/experimental/monitor.cpp 11c06ae 
>   src/tools/balooctl/main.cpp 2a6b175 
>   src/tools/balooctl/statuscommand.cpp 1a56c64 
>   src/tools/balooshow/main.cpp f45f2e0 
> 
> Diff: https://git.reviewboard.kde.org/r/128892/diff/
> 
> 
> Testing
> ---
> 
> Unit tests still work, balooctl seems to do something.
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128892: Open baloo lmdb database read-only beside in baloo_file/baloo_file_extractor + balooctl (for some commands) + unit tests

[Vishesh Handa]

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


Ship it!




This is awesome. Ship it.

- Vishesh Handa


On Sept. 11, 2016, 7:19 p.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128892/
> ---
> 
> (Updated Sept. 11, 2016, 7:19 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma, Boudhayan Gupta, and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> At the moment, any application that uses baloo can corrupt the db.
> Now, only the things that need to write to it open it with read-write.
> This only works as long as the library exposes only read-only things like 
> Query/...
> 
> 
> Diffs
> -
> 
>   src/engine/database.h 6ccb2a5 
>   src/engine/database.cpp 6a433c7 
>   src/file/extractor/app.cpp 0ca7276 
>   src/lib/file.cpp cbbc912 
>   src/lib/searchstore.cpp 060a4fd 
>   src/lib/taglistjob.cpp 76ac8ff 
>   src/qml/experimental/monitor.cpp 11c06ae 
>   src/tools/balooctl/main.cpp 2a6b175 
>   src/tools/balooctl/statuscommand.cpp 1a56c64 
>   src/tools/balooshow/main.cpp f45f2e0 
> 
> Diff: https://git.reviewboard.kde.org/r/128892/diff/
> 
> 
> Testing
> ---
> 
> Unit tests still work, balooctl seems to do something.
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128893: Fix sorted insert (aka flat_map like insert).

[Christoph Cullmann]

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

(Updated Sept. 11, 2016, 4:40 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Boudhayan Gupta.


Changes
---

Submitted with commit 6e5b41e88d92c90df8e54d99163cea08f17d0554 by Christoph 
Cullmann to branch master.


Repository: baloo


Description
---

Old code was plain wrong:

-auto it = std::upper_bound(subDocs.begin(), subDocs.end(), id);
-
-// Merge the id if it does not
-auto prev = it - 1;
-if (*prev != id) {
-subDocs.insert(it, id);
-}


=> you deref begin()-1 in my test case

=> BAM ;)

valgrind backtrace for old code (moved it to template method)

0
PASS   : DocumentUrlDBTest::testGetId()
it == begin 1
==22283== Invalid read of size 8
==22283==at 0x406F20: void Baloo::sortedIdInsert >, unsigned long 
long>(std::vector >&, 
unsigned long long const&) (idutils.h:101)
==22283==by 0x406965: DocumentUrlDBTest::testSortedIdInsert() 
(documenturldbtest.cpp:158)
==22283==by 0x404DD9: DocumentUrlDBTest::qt_static_metacall(QObject*, 
QMetaObject::Call, int, void**) (documenturldbtest.moc:99)
==22283==by 0x57F90BD: QMetaMethod::invoke(QObject*, Qt::ConnectionType, 
QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, 
QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, 
QGenericArgument, QGenericArgument, QGenericArgument) const (in 
/usr/lib/libQt5Core.so.5.7.0)
==22283==by 0x4E489D6: ??? (in /usr/lib/libQt5Test.so.5.7.0)
==22283==by 0x4E49405: ??? (in /usr/lib/libQt5Test.so.5.7.0)
==22283==by 0x4E49A51: ??? (in /usr/lib/libQt5Test.so.5.7.0)
==22283==by 0x4E49F60: QTest::qExec(QObject*, int, char**) (in 
/usr/lib/libQt5Test.so.5.7.0)
==22283==by 0x404CF1: main (documenturldbtest.cpp:167)
==22283==  Address 0xbf25418 is 8 bytes before a block of size 8 alloc'd
==22283==at 0x4C2A0FC: operator new(unsigned long) (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==22283==by 0x40AF63: __gnu_cxx::new_allocator::allocate(unsigned long, void const*) (new_allocator.h:104)
==22283==by 0x40AD46: std::allocator_traits >::allocate(std::allocator&, unsigned long) 
(alloc_traits.h:416)
==22283==by 0x40A171: std::_Vector_base >::_M_allocate(unsigned long) 
(stl_vector.h:170)
==22283==by 0x409151: void std::vector >::_M_emplace_back_aux(unsigned long long&&) (vector.tcc:412)
==22283==by 0x40886C: void std::vector >::emplace_back(unsigned 
long long&&) (vector.tcc:101)
==22283==by 0x406E55: std::vector >::push_back(unsigned long long&&) 
(stl_vector.h:933)
==22283==by 0x40694A: DocumentUrlDBTest::testSortedIdInsert() 
(documenturldbtest.cpp:155)
==22283==by 0x404DD9: DocumentUrlDBTest::qt_static_metacall(QObject*, 
QMetaObject::Call, int, void**) (documenturldbtest.moc:99)
==22283==by 0x57F90BD: QMetaMethod::invoke(QObject*, Qt::ConnectionType, 
QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, 
QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, 
QGenericArgument, QGenericArgument, QGenericArgument) const (in 
/usr/lib/libQt5Core.so.5.7.0)
==22283==by 0x4E489D6: ??? (in /usr/lib/libQt5Test.so.5.7.0)
==22283==by 0x4E49405: ??? (in /usr/lib/libQt5Test.so.5.7.0)
==22283==


Bug report:

https://bugs.kde.org/show_bug.cgi?id=367991


Diffs
-

  autotests/unit/engine/documenturldbtest.cpp 448821b 
  src/engine/documenturldb.cpp 5083e7a 
  src/engine/idutils.h cc7da9c 
  src/engine/writetransaction.cpp 3808970 

Diff: https://git.reviewboard.kde.org/r/128893/diff/


Testing
---

Wrote test, valgrind shows error (or you get segfault, depending on luck) with 
old code, new one works.


Thanks,

Christoph Cullmann

Re: Review Request 128893: Fix sorted insert (aka flat_map like insert).

[Dominik Haumann]

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


Ship it!




Looks good to me.

- Dominik Haumann


On Sept. 11, 2016, 8:35 p.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128893/
> ---
> 
> (Updated Sept. 11, 2016, 8:35 p.m.)
> 
> 
> Review request for KDE Frameworks and Boudhayan Gupta.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Old code was plain wrong:
> 
> -auto it = std::upper_bound(subDocs.begin(), subDocs.end(), id);
> -
> -// Merge the id if it does not
> -auto prev = it - 1;
> -if (*prev != id) {
> -subDocs.insert(it, id);
> -}
> 
> 
> => you deref begin()-1 in my test case
> 
> => BAM ;)
> 
> valgrind backtrace for old code (moved it to template method)
> 
> 0
> PASS   : DocumentUrlDBTest::testGetId()
> it == begin 1
> ==22283== Invalid read of size 8
> ==22283==at 0x406F20: void Baloo::sortedIdInsert long long, std::allocator >, unsigned long 
> long>(std::vector >&, 
> unsigned long long const&) (idutils.h:101)
> ==22283==by 0x406965: DocumentUrlDBTest::testSortedIdInsert() 
> (documenturldbtest.cpp:158)
> ==22283==by 0x404DD9: DocumentUrlDBTest::qt_static_metacall(QObject*, 
> QMetaObject::Call, int, void**) (documenturldbtest.moc:99)
> ==22283==by 0x57F90BD: QMetaMethod::invoke(QObject*, Qt::ConnectionType, 
> QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, 
> QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, 
> QGenericArgument, QGenericArgument, QGenericArgument) const (in 
> /usr/lib/libQt5Core.so.5.7.0)
> ==22283==by 0x4E489D6: ??? (in /usr/lib/libQt5Test.so.5.7.0)
> ==22283==by 0x4E49405: ??? (in /usr/lib/libQt5Test.so.5.7.0)
> ==22283==by 0x4E49A51: ??? (in /usr/lib/libQt5Test.so.5.7.0)
> ==22283==by 0x4E49F60: QTest::qExec(QObject*, int, char**) (in 
> /usr/lib/libQt5Test.so.5.7.0)
> ==22283==by 0x404CF1: main (documenturldbtest.cpp:167)
> ==22283==  Address 0xbf25418 is 8 bytes before a block of size 8 alloc'd
> ==22283==at 0x4C2A0FC: operator new(unsigned long) (in 
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==22283==by 0x40AF63: __gnu_cxx::new_allocator long>::allocate(unsigned long, void const*) (new_allocator.h:104)
> ==22283==by 0x40AD46: std::allocator_traits long> >::allocate(std::allocator&, unsigned long) 
> (alloc_traits.h:416)
> ==22283==by 0x40A171: std::_Vector_base std::allocator >::_M_allocate(unsigned long) 
> (stl_vector.h:170)
> ==22283==by 0x409151: void std::vector std::allocator >::_M_emplace_back_aux long>(unsigned long long&&) (vector.tcc:412)
> ==22283==by 0x40886C: void std::vector std::allocator >::emplace_back long>(unsigned long long&&) (vector.tcc:101)
> ==22283==by 0x406E55: std::vector std::allocator >::push_back(unsigned long long&&) 
> (stl_vector.h:933)
> ==22283==by 0x40694A: DocumentUrlDBTest::testSortedIdInsert() 
> (documenturldbtest.cpp:155)
> ==22283==by 0x404DD9: DocumentUrlDBTest::qt_static_metacall(QObject*, 
> QMetaObject::Call, int, void**) (documenturldbtest.moc:99)
> ==22283==by 0x57F90BD: QMetaMethod::invoke(QObject*, Qt::ConnectionType, 
> QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, 
> QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, 
> QGenericArgument, QGenericArgument, QGenericArgument) const (in 
> /usr/lib/libQt5Core.so.5.7.0)
> ==22283==by 0x4E489D6: ??? (in /usr/lib/libQt5Test.so.5.7.0)
> ==22283==by 0x4E49405: ??? (in /usr/lib/libQt5Test.so.5.7.0)
> ==22283==
> 
> 
> Bug report:
> 
> https://bugs.kde.org/show_bug.cgi?id=367991
> 
> 
> Diffs
> -
> 
>   autotests/unit/engine/documenturldbtest.cpp 448821b 
>   src/engine/documenturldb.cpp 5083e7a 
>   src/engine/idutils.h cc7da9c 
>   src/engine/writetransaction.cpp 3808970 
> 
> Diff: https://git.reviewboard.kde.org/r/128893/diff/
> 
> 
> Testing
> ---
> 
> Wrote test, valgrind shows error (or you get segfault, depending on luck) 
> with old code, new one works.
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128893: Fix sorted insert (aka flat_map like insert).

[Christoph Cullmann]

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

(Updated Sept. 11, 2016, 8:35 p.m.)


Review request for KDE Frameworks and Boudhayan Gupta.


Changes
---

Better unit test, check that result is OK.


Repository: baloo


Description
---

Old code was plain wrong:

-auto it = std::upper_bound(subDocs.begin(), subDocs.end(), id);
-
-// Merge the id if it does not
-auto prev = it - 1;
-if (*prev != id) {
-subDocs.insert(it, id);
-}


=> you deref begin()-1 in my test case

=> BAM ;)

valgrind backtrace for old code (moved it to template method)

0
PASS   : DocumentUrlDBTest::testGetId()
it == begin 1
==22283== Invalid read of size 8
==22283==at 0x406F20: void Baloo::sortedIdInsert >, unsigned long 
long>(std::vector >&, 
unsigned long long const&) (idutils.h:101)
==22283==by 0x406965: DocumentUrlDBTest::testSortedIdInsert() 
(documenturldbtest.cpp:158)
==22283==by 0x404DD9: DocumentUrlDBTest::qt_static_metacall(QObject*, 
QMetaObject::Call, int, void**) (documenturldbtest.moc:99)
==22283==by 0x57F90BD: QMetaMethod::invoke(QObject*, Qt::ConnectionType, 
QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, 
QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, 
QGenericArgument, QGenericArgument, QGenericArgument) const (in 
/usr/lib/libQt5Core.so.5.7.0)
==22283==by 0x4E489D6: ??? (in /usr/lib/libQt5Test.so.5.7.0)
==22283==by 0x4E49405: ??? (in /usr/lib/libQt5Test.so.5.7.0)
==22283==by 0x4E49A51: ??? (in /usr/lib/libQt5Test.so.5.7.0)
==22283==by 0x4E49F60: QTest::qExec(QObject*, int, char**) (in 
/usr/lib/libQt5Test.so.5.7.0)
==22283==by 0x404CF1: main (documenturldbtest.cpp:167)
==22283==  Address 0xbf25418 is 8 bytes before a block of size 8 alloc'd
==22283==at 0x4C2A0FC: operator new(unsigned long) (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==22283==by 0x40AF63: __gnu_cxx::new_allocator::allocate(unsigned long, void const*) (new_allocator.h:104)
==22283==by 0x40AD46: std::allocator_traits >::allocate(std::allocator&, unsigned long) 
(alloc_traits.h:416)
==22283==by 0x40A171: std::_Vector_base >::_M_allocate(unsigned long) 
(stl_vector.h:170)
==22283==by 0x409151: void std::vector >::_M_emplace_back_aux(unsigned long long&&) (vector.tcc:412)
==22283==by 0x40886C: void std::vector >::emplace_back(unsigned 
long long&&) (vector.tcc:101)
==22283==by 0x406E55: std::vector >::push_back(unsigned long long&&) 
(stl_vector.h:933)
==22283==by 0x40694A: DocumentUrlDBTest::testSortedIdInsert() 
(documenturldbtest.cpp:155)
==22283==by 0x404DD9: DocumentUrlDBTest::qt_static_metacall(QObject*, 
QMetaObject::Call, int, void**) (documenturldbtest.moc:99)
==22283==by 0x57F90BD: QMetaMethod::invoke(QObject*, Qt::ConnectionType, 
QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, 
QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, 
QGenericArgument, QGenericArgument, QGenericArgument) const (in 
/usr/lib/libQt5Core.so.5.7.0)
==22283==by 0x4E489D6: ??? (in /usr/lib/libQt5Test.so.5.7.0)
==22283==by 0x4E49405: ??? (in /usr/lib/libQt5Test.so.5.7.0)
==22283==


Bug report:

https://bugs.kde.org/show_bug.cgi?id=367991


Diffs (updated)
-

  autotests/unit/engine/documenturldbtest.cpp 448821b 
  src/engine/documenturldb.cpp 5083e7a 
  src/engine/idutils.h cc7da9c 
  src/engine/writetransaction.cpp 3808970 

Diff: https://git.reviewboard.kde.org/r/128893/diff/


Testing
---

Wrote test, valgrind shows error (or you get segfault, depending on luck) with 
old code, new one works.


Thanks,

Christoph Cullmann

Re: Review Request 128893: Fix sorted insert (aka flat_map like insert).

[Christoph Cullmann]

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

(Updated Sept. 11, 2016, 8:27 p.m.)


Review request for KDE Frameworks and Boudhayan Gupta.


Repository: baloo


Description (updated)
---

Old code was plain wrong:

-auto it = std::upper_bound(subDocs.begin(), subDocs.end(), id);
-
-// Merge the id if it does not
-auto prev = it - 1;
-if (*prev != id) {
-subDocs.insert(it, id);
-}


=> you deref begin()-1 in my test case

=> BAM ;)

valgrind backtrace for old code (moved it to template method)

0
PASS   : DocumentUrlDBTest::testGetId()
it == begin 1
==22283== Invalid read of size 8
==22283==at 0x406F20: void Baloo::sortedIdInsert >, unsigned long 
long>(std::vector >&, 
unsigned long long const&) (idutils.h:101)
==22283==by 0x406965: DocumentUrlDBTest::testSortedIdInsert() 
(documenturldbtest.cpp:158)
==22283==by 0x404DD9: DocumentUrlDBTest::qt_static_metacall(QObject*, 
QMetaObject::Call, int, void**) (documenturldbtest.moc:99)
==22283==by 0x57F90BD: QMetaMethod::invoke(QObject*, Qt::ConnectionType, 
QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, 
QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, 
QGenericArgument, QGenericArgument, QGenericArgument) const (in 
/usr/lib/libQt5Core.so.5.7.0)
==22283==by 0x4E489D6: ??? (in /usr/lib/libQt5Test.so.5.7.0)
==22283==by 0x4E49405: ??? (in /usr/lib/libQt5Test.so.5.7.0)
==22283==by 0x4E49A51: ??? (in /usr/lib/libQt5Test.so.5.7.0)
==22283==by 0x4E49F60: QTest::qExec(QObject*, int, char**) (in 
/usr/lib/libQt5Test.so.5.7.0)
==22283==by 0x404CF1: main (documenturldbtest.cpp:167)
==22283==  Address 0xbf25418 is 8 bytes before a block of size 8 alloc'd
==22283==at 0x4C2A0FC: operator new(unsigned long) (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==22283==by 0x40AF63: __gnu_cxx::new_allocator::allocate(unsigned long, void const*) (new_allocator.h:104)
==22283==by 0x40AD46: std::allocator_traits >::allocate(std::allocator&, unsigned long) 
(alloc_traits.h:416)
==22283==by 0x40A171: std::_Vector_base >::_M_allocate(unsigned long) 
(stl_vector.h:170)
==22283==by 0x409151: void std::vector >::_M_emplace_back_aux(unsigned long long&&) (vector.tcc:412)
==22283==by 0x40886C: void std::vector >::emplace_back(unsigned 
long long&&) (vector.tcc:101)
==22283==by 0x406E55: std::vector >::push_back(unsigned long long&&) 
(stl_vector.h:933)
==22283==by 0x40694A: DocumentUrlDBTest::testSortedIdInsert() 
(documenturldbtest.cpp:155)
==22283==by 0x404DD9: DocumentUrlDBTest::qt_static_metacall(QObject*, 
QMetaObject::Call, int, void**) (documenturldbtest.moc:99)
==22283==by 0x57F90BD: QMetaMethod::invoke(QObject*, Qt::ConnectionType, 
QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, 
QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, 
QGenericArgument, QGenericArgument, QGenericArgument) const (in 
/usr/lib/libQt5Core.so.5.7.0)
==22283==by 0x4E489D6: ??? (in /usr/lib/libQt5Test.so.5.7.0)
==22283==by 0x4E49405: ??? (in /usr/lib/libQt5Test.so.5.7.0)
==22283==


Diffs
-

  autotests/unit/engine/documenturldbtest.cpp 448821b 
  src/engine/documenturldb.cpp 5083e7a 
  src/engine/idutils.h cc7da9c 
  src/engine/writetransaction.cpp 3808970 

Diff: https://git.reviewboard.kde.org/r/128893/diff/


Testing
---

Wrote test, valgrind shows error (or you get segfault, depending on luck) with 
old code, new one works.


Thanks,

Christoph Cullmann

Re: Review Request 128893: Fix sorted insert (aka flat_map like insert).

[Christoph Cullmann]

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

(Updated Sept. 11, 2016, 8:27 p.m.)


Review request for KDE Frameworks and Boudhayan Gupta.


Repository: baloo


Description (updated)
---

Old code was plain wrong:

-auto it = std::upper_bound(subDocs.begin(), subDocs.end(), id);
-
-// Merge the id if it does not
-auto prev = it - 1;
-if (*prev != id) {
-subDocs.insert(it, id);
-}


=> you deref begin()-1 in my test case

=> BAM ;)

valgrind backtrace for old code (moved it to template method)

0
PASS   : DocumentUrlDBTest::testGetId()
it == begin 1
==22283== Invalid read of size 8
==22283==at 0x406F20: void Baloo::sortedIdInsert >, unsigned long 
long>(std::vector >&, 
unsigned long long const&) (idutils.h:101)
==22283==by 0x406965: DocumentUrlDBTest::testSortedIdInsert() 
(documenturldbtest.cpp:158)
==22283==by 0x404DD9: DocumentUrlDBTest::qt_static_metacall(QObject*, 
QMetaObject::Call, int, void**) (documenturldbtest.moc:99)
==22283==by 0x57F90BD: QMetaMethod::invoke(QObject*, Qt::ConnectionType, 
QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, 
QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, 
QGenericArgument, QGenericArgument, QGenericArgument) const (in 
/usr/lib/libQt5Core.so.5.7.0)
==22283==by 0x4E489D6: ??? (in /usr/lib/libQt5Test.so.5.7.0)
==22283==by 0x4E49405: ??? (in /usr/lib/libQt5Test.so.5.7.0)
==22283==by 0x4E49A51: ??? (in /usr/lib/libQt5Test.so.5.7.0)
==22283==by 0x4E49F60: QTest::qExec(QObject*, int, char**) (in 
/usr/lib/libQt5Test.so.5.7.0)
==22283==by 0x404CF1: main (documenturldbtest.cpp:167)
==22283==  Address 0xbf25418 is 8 bytes before a block of size 8 alloc'd
==22283==at 0x4C2A0FC: operator new(unsigned long) (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==22283==by 0x40AF63: __gnu_cxx::new_allocator::allocate(unsigned long, void const*) (new_allocator.h:104)
==22283==by 0x40AD46: std::allocator_traits >::allocate(std::allocator&, unsigned long) 
(alloc_traits.h:416)
==22283==by 0x40A171: std::_Vector_base >::_M_allocate(unsigned long) 
(stl_vector.h:170)
==22283==by 0x409151: void std::vector >::_M_emplace_back_aux(unsigned long long&&) (vector.tcc:412)
==22283==by 0x40886C: void std::vector >::emplace_back(unsigned 
long long&&) (vector.tcc:101)
==22283==by 0x406E55: std::vector >::push_back(unsigned long long&&) 
(stl_vector.h:933)
==22283==by 0x40694A: DocumentUrlDBTest::testSortedIdInsert() 
(documenturldbtest.cpp:155)
==22283==by 0x404DD9: DocumentUrlDBTest::qt_static_metacall(QObject*, 
QMetaObject::Call, int, void**) (documenturldbtest.moc:99)
==22283==by 0x57F90BD: QMetaMethod::invoke(QObject*, Qt::ConnectionType, 
QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, 
QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, 
QGenericArgument, QGenericArgument, QGenericArgument) const (in 
/usr/lib/libQt5Core.so.5.7.0)
==22283==by 0x4E489D6: ??? (in /usr/lib/libQt5Test.so.5.7.0)
==22283==by 0x4E49405: ??? (in /usr/lib/libQt5Test.so.5.7.0)
==22283==


Bug report:

https://bugs.kde.org/show_bug.cgi?id=367991


Diffs
-

  autotests/unit/engine/documenturldbtest.cpp 448821b 
  src/engine/documenturldb.cpp 5083e7a 
  src/engine/idutils.h cc7da9c 
  src/engine/writetransaction.cpp 3808970 

Diff: https://git.reviewboard.kde.org/r/128893/diff/


Testing
---

Wrote test, valgrind shows error (or you get segfault, depending on luck) with 
old code, new one works.


Thanks,

Christoph Cullmann

Review Request 128893: Fix sorted insert (aka flat_map like insert).

[Christoph Cullmann]

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

Review request for KDE Frameworks and Boudhayan Gupta.


Repository: baloo


Description
---

Old code was plain wrong:

-auto it = std::upper_bound(subDocs.begin(), subDocs.end(), id);
-
-// Merge the id if it does not
-auto prev = it - 1;
-if (*prev != id) {
-subDocs.insert(it, id);
-}


=> you deref begin()-1 in my test case

=> BAM ;)


Diffs
-

  autotests/unit/engine/documenturldbtest.cpp 448821b 
  src/engine/documenturldb.cpp 5083e7a 
  src/engine/idutils.h cc7da9c 
  src/engine/writetransaction.cpp 3808970 

Diff: https://git.reviewboard.kde.org/r/128893/diff/


Testing
---

Wrote test, valgrind shows error (or you get segfault, depending on luck) with 
old code, new one works.


Thanks,

Christoph Cullmann

Re: Taking over maintainership of Baloo

[Christoph Cullmann]

Hi,

> Hi,
> 
> indeed I also did take a look at the bug list, and out of the 200
> bugs, without much exaggeration almost all of them are crashes.
> 
> In fact, I believe it's important to CC the plasma-devel mailing list
> here: Given we are about to release a LTS Plasma 5.8, and baloo is
> tightly integrated into Plasma, Plasma will still contain lots and
> lots of crashes.
> 
> Maybe the Plasma people are also interested in taking e.g. 2-3 days to
> look into this.
> 
> Greetings,
> Dominik
> 
> PS: Many crashes are probably due to missing error checks, e.g.: in
> postingdb.cpp:
> 
>int rc = mdb_get(m_txn, m_dbi, &key, &val);
>if (rc == MDB_NOTFOUND) {
>return 0;
>}
>Q_ASSERT_X(rc == 0, "PostingDB::iter", mdb_strerror(rc));
> 
>return new DBPostingIterator(val.mv_data, val.mv_size);
> 
> Looking into the API documentation, mdb_get() [1] says:
>  Returns: A non-zero error value on failure and 0 on success. Some
> possible errors are:
> MDB_NOTFOUND - the key was not in the database.
> EINVAL - an invalid parameter was specified.
> 
> Note the "Some possible errors", there are many many more. The code
> should probably be if (rc != 0) return 0; etc. Baloo has tons of this
> bugs.
> 
> [1]
> http://104.237.133.194/doc/group__mdb.html#ga8bf10cd91d3f3a83a34d04ce6b07992d

I triaged now > 100 crash bugs, more or less everything boils down to the
non-existing error handling or out of space.

Greetings
Christoph

-- 
- Dr.-Ing. Christoph Cullmann -
AbsInt Angewandte Informatik GmbH  Email: cullm...@absint.com
Science Park 1 Tel:   +49-681-38360-22
66123 Saarbrücken  Fax:   +49-681-38360-20
GERMANYWWW:   http://www.AbsInt.com

Geschäftsführung: Dr.-Ing. Christian Ferdinand
Eingetragen im Handelsregister des Amtsgerichts Saarbrücken, HRB 11234

Re: Review Request 128892: Open baloo lmdb database read-only beside in baloo_file/baloo_file_extractor + balooctl (for some commands) + unit tests

[Christoph Cullmann]

> On Sept. 11, 2016, 7:19 p.m., Boudhayan Gupta wrote:
> > Can you add Vishesh and poke him if possible? This seems like a gigantic 
> > patch and I'm not sure I understand all of it right now.

Hi, I can poke him, sure.
Given there is no documentation what the things shall do, I am not 100% sure if 
that is perfect, myself.
On the other side: ATM all things open the database read-write, which imho is 
VERY bad.


- Christoph


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


On Sept. 11, 2016, 7:19 p.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128892/
> ---
> 
> (Updated Sept. 11, 2016, 7:19 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma, Boudhayan Gupta, and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> At the moment, any application that uses baloo can corrupt the db.
> Now, only the things that need to write to it open it with read-write.
> This only works as long as the library exposes only read-only things like 
> Query/...
> 
> 
> Diffs
> -
> 
>   src/engine/database.h 6ccb2a5 
>   src/engine/database.cpp 6a433c7 
>   src/file/extractor/app.cpp 0ca7276 
>   src/lib/file.cpp cbbc912 
>   src/lib/searchstore.cpp 060a4fd 
>   src/lib/taglistjob.cpp 76ac8ff 
>   src/qml/experimental/monitor.cpp 11c06ae 
>   src/tools/balooctl/main.cpp 2a6b175 
>   src/tools/balooctl/statuscommand.cpp 1a56c64 
>   src/tools/balooshow/main.cpp f45f2e0 
> 
> Diff: https://git.reviewboard.kde.org/r/128892/diff/
> 
> 
> Testing
> ---
> 
> Unit tests still work, balooctl seems to do something.
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128892: Open baloo lmdb database read-only beside in baloo_file/baloo_file_extractor + balooctl (for some commands) + unit tests

[Boudhayan Gupta]

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



Can you add Vishesh and poke him if possible? This seems like a gigantic patch 
and I'm not sure I understand all of it right now.

- Boudhayan Gupta


On Sept. 12, 2016, 12:42 a.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128892/
> ---
> 
> (Updated Sept. 12, 2016, 12:42 a.m.)
> 
> 
> Review request for KDE Frameworks, Plasma and Boudhayan Gupta.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> At the moment, any application that uses baloo can corrupt the db.
> Now, only the things that need to write to it open it with read-write.
> This only works as long as the library exposes only read-only things like 
> Query/...
> 
> 
> Diffs
> -
> 
>   src/engine/database.h 6ccb2a5 
>   src/engine/database.cpp 6a433c7 
>   src/file/extractor/app.cpp 0ca7276 
>   src/lib/file.cpp cbbc912 
>   src/lib/searchstore.cpp 060a4fd 
>   src/lib/taglistjob.cpp 76ac8ff 
>   src/qml/experimental/monitor.cpp 11c06ae 
>   src/tools/balooctl/main.cpp 2a6b175 
>   src/tools/balooctl/statuscommand.cpp 1a56c64 
>   src/tools/balooshow/main.cpp f45f2e0 
> 
> Diff: https://git.reviewboard.kde.org/r/128892/diff/
> 
> 
> Testing
> ---
> 
> Unit tests still work, balooctl seems to do something.
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128891: Transaction not created => don't try to abort them

[Boudhayan Gupta]

> On Sept. 12, 2016, 12:25 a.m., Boudhayan Gupta wrote:
> > LGTM.
> > 
> > Long term I plan to replace all of this with liblmdb++, but that may or may 
> > not happen soon-ish.
> 
> Christoph Cullmann wrote:
> Hi, lmdb++ is IMHO no good idea, look at their commits, last one 7 months 
> ago, only 2 people ever did anything.
> https://github.com/bendiken/lmdbxx
> No good idea to rely on that. (and it has exactly zero unit tests)

In that case, I'll have to tidy this code up and make a clean wrapper around 
the C API. *sigh*


- Boudhayan


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


On Sept. 12, 2016, 12:43 a.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128891/
> ---
> 
> (Updated Sept. 12, 2016, 12:43 a.m.)
> 
> 
> Review request for KDE Frameworks and Boudhayan Gupta.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Transaction not created => don't try to abort them, avoid wrong usage of lmdb 
> api
> 
> 
> Diffs
> -
> 
>   src/engine/database.cpp 6a433c7 
> 
> Diff: https://git.reviewboard.kde.org/r/128891/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128891: Transaction not created => don't try to abort them

[Christoph Cullmann]

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

(Updated Sept. 11, 2016, 7:13 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Boudhayan Gupta.


Changes
---

Submitted with commit 54f7363048c7db41f63c85f637911a5598c30e9e by Christoph 
Cullmann to branch master.


Repository: baloo


Description
---

Transaction not created => don't try to abort them, avoid wrong usage of lmdb 
api


Diffs
-

  src/engine/database.cpp 6a433c7 

Diff: https://git.reviewboard.kde.org/r/128891/diff/


Testing
---


Thanks,

Christoph Cullmann

Review Request 128892: Open baloo lmdb database read-only beside in baloo_file/baloo_file_extractor + balooctl (for some commands) + unit tests

[Christoph Cullmann]

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

Review request for KDE Frameworks, Plasma and Boudhayan Gupta.


Repository: baloo


Description
---

At the moment, any application that uses baloo can corrupt the db.
Now, only the things that need to write to it open it with read-write.
This only works as long as the library exposes only read-only things like 
Query/...


Diffs
-

  src/engine/database.h 6ccb2a5 
  src/engine/database.cpp 6a433c7 
  src/file/extractor/app.cpp 0ca7276 
  src/lib/file.cpp cbbc912 
  src/lib/searchstore.cpp 060a4fd 
  src/lib/taglistjob.cpp 76ac8ff 
  src/qml/experimental/monitor.cpp 11c06ae 
  src/tools/balooctl/main.cpp 2a6b175 
  src/tools/balooctl/statuscommand.cpp 1a56c64 
  src/tools/balooshow/main.cpp f45f2e0 

Diff: https://git.reviewboard.kde.org/r/128892/diff/


Testing
---

Unit tests still work, balooctl seems to do something.


Thanks,

Christoph Cullmann

Re: Review Request 128891: Transaction not created => don't try to abort them

[Christoph Cullmann]

> On Sept. 11, 2016, 6:55 p.m., Boudhayan Gupta wrote:
> > LGTM.
> > 
> > Long term I plan to replace all of this with liblmdb++, but that may or may 
> > not happen soon-ish.

Hi, lmdb++ is IMHO no good idea, look at their commits, last one 7 months ago, 
only 2 people ever did anything.
https://github.com/bendiken/lmdbxx
No good idea to rely on that. (and it has exactly zero unit tests)


- Christoph


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


On Sept. 11, 2016, 6:51 p.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128891/
> ---
> 
> (Updated Sept. 11, 2016, 6:51 p.m.)
> 
> 
> Review request for KDE Frameworks and Boudhayan Gupta.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Transaction not created => don't try to abort them, avoid wrong usage of lmdb 
> api
> 
> 
> Diffs
> -
> 
>   src/engine/database.cpp 6a433c7 
> 
> Diff: https://git.reviewboard.kde.org/r/128891/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128891: Transaction not created => don't try to abort them

[Boudhayan Gupta]

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


Ship it!




LGTM.

Long term I plan to replace all of this with liblmdb++, but that may or may not 
happen soon-ish.

- Boudhayan Gupta


On Sept. 12, 2016, 12:21 a.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128891/
> ---
> 
> (Updated Sept. 12, 2016, 12:21 a.m.)
> 
> 
> Review request for KDE Frameworks and Boudhayan Gupta.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Transaction not created => don't try to abort them, avoid wrong usage of lmdb 
> api
> 
> 
> Diffs
> -
> 
>   src/engine/database.cpp 6a433c7 
> 
> Diff: https://git.reviewboard.kde.org/r/128891/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Review Request 128891: Transaction not created => don't try to abort them

[Christoph Cullmann]

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

Review request for KDE Frameworks and Boudhayan Gupta.


Repository: baloo


Description
---

Transaction not created => don't try to abort them, avoid wrong usage of lmdb 
api


Diffs
-

  src/engine/database.cpp 6a433c7 

Diff: https://git.reviewboard.kde.org/r/128891/diff/


Testing
---


Thanks,

Christoph Cullmann

Re: Review Request 128890: Make e.g. Baloo::Query thread safe.

[Christoph Cullmann]

> On Sept. 11, 2016, 6:32 p.m., Anthony Fieroni wrote:
> > src/engine/database.cpp, line 128
> > 
> >
> > It's needed *m_env = 0;*
> > Or you can add method close to call it in all way.
> 
> Christoph Cullmann wrote:
> I don't understand the comment ;)
> 1) m_env != null means valid atm, therefore I can call here close
> 2) after this, the destructor is over, no need to null it, if you access 
> the database object after/during destruction, all is lost.
> 
> Anthony Fieroni wrote:
> Missing m_env = 0 after closing call. It can lead crash it future use.

Ah ;=) I see, I misread the line!
Thanks


- Christoph


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


On Sept. 11, 2016, 6:27 p.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128890/
> ---
> 
> (Updated Sept. 11, 2016, 6:27 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma and Boudhayan Gupta.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> lmdb itself is thread safe (e.g. you can use the same env in multiple 
> threads).
> Unfortunately, the Baloo::Database itself not, as open() might race against 
> other open calls (we have one unique db object in baloo).
> 
> => add non-recursive mutex (recursive mutex not needed, one just must avoid 
> to call isOpen() or path() inside open, that is done, else no unit test 
> works).
> 
> 
> Diffs
> -
> 
>   src/engine/database.h e3bb175 
>   src/engine/database.cpp ec7ae2e 
> 
> Diff: https://git.reviewboard.kde.org/r/128890/diff/
> 
> 
> Testing
> ---
> 
> Unit tests still work.
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128890: Make e.g. Baloo::Query thread safe.

[Anthony Fieroni]

> On Септ. 11, 2016, 9:32 след обяд, Anthony Fieroni wrote:
> > src/engine/database.cpp, line 128
> > 
> >
> > It's needed *m_env = 0;*
> > Or you can add method close to call it in all way.
> 
> Christoph Cullmann wrote:
> I don't understand the comment ;)
> 1) m_env != null means valid atm, therefore I can call here close
> 2) after this, the destructor is over, no need to null it, if you access 
> the database object after/during destruction, all is lost.

Missing m_env = 0 after closing call. It can lead crash it future use.


- Anthony


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


On Септ. 11, 2016, 9:27 след обяд, Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128890/
> ---
> 
> (Updated Септ. 11, 2016, 9:27 след обяд)
> 
> 
> Review request for KDE Frameworks, Plasma and Boudhayan Gupta.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> lmdb itself is thread safe (e.g. you can use the same env in multiple 
> threads).
> Unfortunately, the Baloo::Database itself not, as open() might race against 
> other open calls (we have one unique db object in baloo).
> 
> => add non-recursive mutex (recursive mutex not needed, one just must avoid 
> to call isOpen() or path() inside open, that is done, else no unit test 
> works).
> 
> 
> Diffs
> -
> 
>   src/engine/database.h e3bb175 
>   src/engine/database.cpp ec7ae2e 
> 
> Diff: https://git.reviewboard.kde.org/r/128890/diff/
> 
> 
> Testing
> ---
> 
> Unit tests still work.
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128890: Make e.g. Baloo::Query thread safe.

[Christoph Cullmann]

> On Sept. 11, 2016, 6:32 p.m., Anthony Fieroni wrote:
> > src/engine/database.cpp, line 128
> > 
> >
> > It's needed *m_env = 0;*
> > Or you can add method close to call it in all way.

I don't understand the comment ;)
1) m_env != null means valid atm, therefore I can call here close
2) after this, the destructor is over, no need to null it, if you access the 
database object after/during destruction, all is lost.


- Christoph


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


On Sept. 11, 2016, 6:27 p.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128890/
> ---
> 
> (Updated Sept. 11, 2016, 6:27 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma and Boudhayan Gupta.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> lmdb itself is thread safe (e.g. you can use the same env in multiple 
> threads).
> Unfortunately, the Baloo::Database itself not, as open() might race against 
> other open calls (we have one unique db object in baloo).
> 
> => add non-recursive mutex (recursive mutex not needed, one just must avoid 
> to call isOpen() or path() inside open, that is done, else no unit test 
> works).
> 
> 
> Diffs
> -
> 
>   src/engine/database.h e3bb175 
>   src/engine/database.cpp ec7ae2e 
> 
> Diff: https://git.reviewboard.kde.org/r/128890/diff/
> 
> 
> Testing
> ---
> 
> Unit tests still work.
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128890: Make e.g. Baloo::Query thread safe.

[Anthony Fieroni]

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




src/engine/database.cpp (line 128)


It's needed *m_env = 0;*
Or you can add method close to call it in all way.



src/engine/database.cpp (line 128)





- Anthony Fieroni


On Септ. 11, 2016, 9:27 след обяд, Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128890/
> ---
> 
> (Updated Септ. 11, 2016, 9:27 след обяд)
> 
> 
> Review request for KDE Frameworks, Plasma and Boudhayan Gupta.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> lmdb itself is thread safe (e.g. you can use the same env in multiple 
> threads).
> Unfortunately, the Baloo::Database itself not, as open() might race against 
> other open calls (we have one unique db object in baloo).
> 
> => add non-recursive mutex (recursive mutex not needed, one just must avoid 
> to call isOpen() or path() inside open, that is done, else no unit test 
> works).
> 
> 
> Diffs
> -
> 
>   src/engine/database.h e3bb175 
>   src/engine/database.cpp ec7ae2e 
> 
> Diff: https://git.reviewboard.kde.org/r/128890/diff/
> 
> 
> Testing
> ---
> 
> Unit tests still work.
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128890: Make e.g. Baloo::Query thread safe.

[Kai Uwe Broulik]

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




src/engine/database.h (line 60)


@param


- Kai Uwe Broulik


On Sept. 11, 2016, 6:27 nachm., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128890/
> ---
> 
> (Updated Sept. 11, 2016, 6:27 nachm.)
> 
> 
> Review request for KDE Frameworks, Plasma and Boudhayan Gupta.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> lmdb itself is thread safe (e.g. you can use the same env in multiple 
> threads).
> Unfortunately, the Baloo::Database itself not, as open() might race against 
> other open calls (we have one unique db object in baloo).
> 
> => add non-recursive mutex (recursive mutex not needed, one just must avoid 
> to call isOpen() or path() inside open, that is done, else no unit test 
> works).
> 
> 
> Diffs
> -
> 
>   src/engine/database.h e3bb175 
>   src/engine/database.cpp ec7ae2e 
> 
> Diff: https://git.reviewboard.kde.org/r/128890/diff/
> 
> 
> Testing
> ---
> 
> Unit tests still work.
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128890: Make e.g. Baloo::Query thread safe.

[Christoph Cullmann]

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

(Updated Sept. 11, 2016, 9:27 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, Plasma and Boudhayan Gupta.


Changes
---

Submitted with commit e34da150d82a57cf417a59b8b632b2fecb32a6f7 by Christoph 
Cullmann to branch master.


Repository: baloo


Description
---

lmdb itself is thread safe (e.g. you can use the same env in multiple threads).
Unfortunately, the Baloo::Database itself not, as open() might race against 
other open calls (we have one unique db object in baloo).

=> add non-recursive mutex (recursive mutex not needed, one just must avoid to 
call isOpen() or path() inside open, that is done, else no unit test works).


Diffs
-

  src/engine/database.h e3bb175 
  src/engine/database.cpp ec7ae2e 

Diff: https://git.reviewboard.kde.org/r/128890/diff/


Testing
---

Unit tests still work.


Thanks,

Christoph Cullmann

Re: Review Request 128890: Make e.g. Baloo::Query thread safe.

[Boudhayan Gupta]

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


Ship it!




Ship It!

- Boudhayan Gupta


On Sept. 11, 2016, 11:49 p.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128890/
> ---
> 
> (Updated Sept. 11, 2016, 11:49 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma and Boudhayan Gupta.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> lmdb itself is thread safe (e.g. you can use the same env in multiple 
> threads).
> Unfortunately, the Baloo::Database itself not, as open() might race against 
> other open calls (we have one unique db object in baloo).
> 
> => add non-recursive mutex (recursive mutex not needed, one just must avoid 
> to call isOpen() or path() inside open, that is done, else no unit test 
> works).
> 
> 
> Diffs
> -
> 
>   src/engine/database.h e3bb175 
>   src/engine/database.cpp ec7ae2e 
> 
> Diff: https://git.reviewboard.kde.org/r/128890/diff/
> 
> 
> Testing
> ---
> 
> Unit tests still work.
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Review Request 128890: Make e.g. Baloo::Query thread safe.

[Christoph Cullmann]

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

Review request for KDE Frameworks, Plasma and Boudhayan Gupta.


Repository: baloo


Description
---

lmdb itself is thread safe (e.g. you can use the same env in multiple threads).
Unfortunately, the Baloo::Database itself not, as open() might race against 
other open calls (we have one unique db object in baloo).

=> add non-recursive mutex (recursive mutex not needed, one just must avoid to 
call isOpen() or path() inside open, that is done, else no unit test works).


Diffs
-

  src/engine/database.h e3bb175 
  src/engine/database.cpp ec7ae2e 

Diff: https://git.reviewboard.kde.org/r/128890/diff/


Testing
---

Unit tests still work.


Thanks,

Christoph Cullmann

Re: Taking over maintainership of Baloo

[Dominik Haumann]

Hi,

indeed I also did take a look at the bug list, and out of the 200
bugs, without much exaggeration almost all of them are crashes.

In fact, I believe it's important to CC the plasma-devel mailing list
here: Given we are about to release a LTS Plasma 5.8, and baloo is
tightly integrated into Plasma, Plasma will still contain lots and
lots of crashes.

Maybe the Plasma people are also interested in taking e.g. 2-3 days to
look into this.

Greetings,
Dominik

PS: Many crashes are probably due to missing error checks, e.g.: in
postingdb.cpp:

int rc = mdb_get(m_txn, m_dbi, &key, &val);
if (rc == MDB_NOTFOUND) {
return 0;
}
Q_ASSERT_X(rc == 0, "PostingDB::iter", mdb_strerror(rc));

return new DBPostingIterator(val.mv_data, val.mv_size);

Looking into the API documentation, mdb_get() [1] says:
  Returns: A non-zero error value on failure and 0 on success. Some
possible errors are:
 MDB_NOTFOUND - the key was not in the database.
 EINVAL - an invalid parameter was specified.

Note the "Some possible errors", there are many many more. The code
should probably be if (rc != 0) return 0; etc. Baloo has tons of this
bugs.

[1] 
http://104.237.133.194/doc/group__mdb.html#ga8bf10cd91d3f3a83a34d04ce6b07992d


On Sun, Sep 11, 2016 at 7:29 PM, Christoph Cullmann  wrote:
> Hi,
>
>> Hi,
>>
>> On 11 September 2016 at 22:44, Christoph Cullmann  
>> wrote:
>>> Hi,
>>>
>>> it would be very nice to have some active maintainership there,
>>> given a lot of VERY major crash bugs pile up there since > 1 year.
>>
>> The issue is that neither Pinak nor I can reproduce much of the bugs,
>> making solving them difficult. I'll try harder to nowadays, though ;-)
> I think the main issue is: There is no need to reproduce them, it is clear, 
> that baloo will crash
> on any problem with lmdb, as "no" errors are handled (or lets say 99% of the 
> errors are not handled).
>
> The code is full of Q_ASSERT, but no handling of the return codes beside that.
>
> Actually, I think, in most cases the code shall just catch the error cases 
> and do nothing (or purge data
> until all is fine again) but not like now: crash or assert.
>
> That is not acceptable for something that runs per default ;=)
>
> Just looking at the code, you see things like:
>
> 1) Baloo::Database: needs a mutex, as many threads might call ::open (e.g. in 
> krunner) => corruption, dead
>
> 2) inproper cleanup: I doubt one can  mdb_txn_abort(txn); if already 
> mdb_txn_begin(...) failed
>
> 3) needs everywhere return code checks, e.g. in PostingList 
> PostingDB::get(const QByteArray& term), we have
> tons of bugs about random crashs afterwards: 
> https://bugs.kde.org/show_bug.cgi?id=367480
>
> 4) 32-bit systems supported at all? ATM, after 1GB of indexing, that was it, 
> no more baloo or any other application
> calling any of the accessors of e.g. Query (if you have bad luck).
>
>>
>>> Beside that, could we get some baloo-b...@kde.org list as default assignee
>>> for all baloo bugs instead of "one" person?
>>
>> If we're going to do it this way, why not just make it the default
>> frameworks mailing list, or the frameworks bugs list if there is one?
> I am not sure, if that will make people happy ;=)
>
> Greetings
> Christoph
>
> --
> - Dr.-Ing. Christoph Cullmann -
> AbsInt Angewandte Informatik GmbH  Email: cullm...@absint.com
> Science Park 1 Tel:   +49-681-38360-22
> 66123 Saarbrücken  Fax:   +49-681-38360-20
> GERMANYWWW:   http://www.AbsInt.com
> 
> Geschäftsführung: Dr.-Ing. Christian Ferdinand
> Eingetragen im Handelsregister des Amtsgerichts Saarbrücken, HRB 11234

Re: Taking over maintainership of Baloo

[Boudhayan Gupta]

Hi,

On 11 September 2016 at 22:59, Christoph Cullmann  wrote:
> I think the main issue is: There is no need to reproduce them, it is clear, 
> that baloo will crash
> on any problem with lmdb, as "no" errors are handled (or lets say 99% of the 
> errors are not handled).
>
> The code is full of Q_ASSERT, but no handling of the return codes beside that.

Oh, it gets worse. Q_ASSERTs compile to nothing in release mode so you
don't even have meaningful crashes on those asserts, things just
mysteriously fail elsewhere. I spent a whole bunch of time coding in
actual checks after those asserts because Dolphin would crash when
selecting multiple files with Baloo disabled, and other such magical
bugs.

> Actually, I think, in most cases the code shall just catch the error cases 
> and do nothing (or purge data
> until all is fine again) but not like now: crash or assert.
>
> That is not acceptable for something that runs per default ;=)
>
> Just looking at the code, you see things like:
>
> 1) Baloo::Database: needs a mutex, as many threads might call ::open (e.g. in 
> krunner) => corruption, dead
>
> 2) inproper cleanup: I doubt one can  mdb_txn_abort(txn); if already 
> mdb_txn_begin(...) failed
>
> 3) needs everywhere return code checks, e.g. in PostingList 
> PostingDB::get(const QByteArray& term), we have
> tons of bugs about random crashs afterwards: 
> https://bugs.kde.org/show_bug.cgi?id=367480
>
> 4) 32-bit systems supported at all? ATM, after 1GB of indexing, that was it, 
> no more baloo or any other application
> calling any of the accessors of e.g. Query (if you have bad luck).

If you have the time to fix those things you've listed above, awesome
- I'd love to look at the RRs. Lessens my workload.

Otherwise I'll look at this on the weekends when I have no sysadmin
things to do.

>
>>
>>> Beside that, could we get some baloo-b...@kde.org list as default assignee
>>> for all baloo bugs instead of "one" person?
>>
>> If we're going to do it this way, why not just make it the default
>> frameworks mailing list, or the frameworks bugs list if there is one?
> I am not sure, if that will make people happy ;=)

Let's get other people involved, too ;-)

Thanks,
Boudhayan

Re: Taking over maintainership of Baloo

[Christoph Cullmann]

Hi,

> Hi,
> 
> On 11 September 2016 at 22:44, Christoph Cullmann  wrote:
>> Hi,
>>
>> it would be very nice to have some active maintainership there,
>> given a lot of VERY major crash bugs pile up there since > 1 year.
> 
> The issue is that neither Pinak nor I can reproduce much of the bugs,
> making solving them difficult. I'll try harder to nowadays, though ;-)
I think the main issue is: There is no need to reproduce them, it is clear, 
that baloo will crash
on any problem with lmdb, as "no" errors are handled (or lets say 99% of the 
errors are not handled).

The code is full of Q_ASSERT, but no handling of the return codes beside that.

Actually, I think, in most cases the code shall just catch the error cases and 
do nothing (or purge data
until all is fine again) but not like now: crash or assert.

That is not acceptable for something that runs per default ;=)

Just looking at the code, you see things like:

1) Baloo::Database: needs a mutex, as many threads might call ::open (e.g. in 
krunner) => corruption, dead

2) inproper cleanup: I doubt one can  mdb_txn_abort(txn); if already 
mdb_txn_begin(...) failed

3) needs everywhere return code checks, e.g. in PostingList 
PostingDB::get(const QByteArray& term), we have
tons of bugs about random crashs afterwards: 
https://bugs.kde.org/show_bug.cgi?id=367480

4) 32-bit systems supported at all? ATM, after 1GB of indexing, that was it, no 
more baloo or any other application
calling any of the accessors of e.g. Query (if you have bad luck).

> 
>> Beside that, could we get some baloo-b...@kde.org list as default assignee
>> for all baloo bugs instead of "one" person?
> 
> If we're going to do it this way, why not just make it the default
> frameworks mailing list, or the frameworks bugs list if there is one?
I am not sure, if that will make people happy ;=)

Greetings
Christoph

-- 
- Dr.-Ing. Christoph Cullmann -
AbsInt Angewandte Informatik GmbH  Email: cullm...@absint.com
Science Park 1 Tel:   +49-681-38360-22
66123 Saarbrücken  Fax:   +49-681-38360-20
GERMANYWWW:   http://www.AbsInt.com

Geschäftsführung: Dr.-Ing. Christian Ferdinand
Eingetragen im Handelsregister des Amtsgerichts Saarbrücken, HRB 11234

Re: Taking over maintainership of Baloo

[Boudhayan Gupta]

Hi,

On 11 September 2016 at 22:44, Christoph Cullmann  wrote:
> Hi,
>
> it would be very nice to have some active maintainership there,
> given a lot of VERY major crash bugs pile up there since > 1 year.

The issue is that neither Pinak nor I can reproduce much of the bugs,
making solving them difficult. I'll try harder to nowadays, though ;-)

> Beside that, could we get some baloo-b...@kde.org list as default assignee
> for all baloo bugs instead of "one" person?

If we're going to do it this way, why not just make it the default
frameworks mailing list, or the frameworks bugs list if there is one?

> Greetings
> Christoph

-- Boudhayan

>
> - Am 11. Sep 2016 um 19:08 schrieb Boudhayan Gupta bgu...@kde.org:
>
>> Hi all,
>>
>> Since Vishesh effectively stopped maintaining Baloo full-time, mostly
>> Pinak and to some extent I have been looking over bugs and reviewing
>> RRs pertaining to Baloo. If there are no objections to the same
>> (including if Pinak declines to either co-maintain it or would rather
>> maintain it alone, sans me), I'd like to officially take over
>> maintainership of Baloo, co-maintaining it with Pinak.
>>
>> Thanks,
>> Boudhayan
>
> --
> - Dr.-Ing. Christoph Cullmann -
> AbsInt Angewandte Informatik GmbH  Email: cullm...@absint.com
> Science Park 1 Tel:   +49-681-38360-22
> 66123 Saarbrücken  Fax:   +49-681-38360-20
> GERMANYWWW:   http://www.AbsInt.com
> 
> Geschäftsführung: Dr.-Ing. Christian Ferdinand
> Eingetragen im Handelsregister des Amtsgerichts Saarbrücken, HRB 11234

Re: Review Request 128888: Improve epub extractor, less segfaults

[Christoph Cullmann]

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

(Updated Sept. 11, 2016, 7:17 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, Boudhayan Gupta and Pinak Ahuja.


Changes
---

Submitted with commit 47f6e57b2fa3768feb4f1f4a2cd3ce46660d90f2 by Christoph 
Cullmann to branch master.


Repository: kfilemetadata


Description
---

Improve epub extractor:

1) check for more nullpointers (e.g. data can be null for some fields, 
iterators, ...)
2) actually close the epub file again at all
3) iterator seems to handle clink as stated in docs, fix double free

e.g. see bug 361727
could be the double freed clink in the last iterator

https://bugs.kde.org/show_bug.cgi?id=361727


Diffs
-

  src/extractors/epubextractor.cpp 88a4caa 

Diff: https://git.reviewboard.kde.org/r/12/diff/


Testing
---

Unit test still works, no idea how to craft files broken enough to trigger the 
faults


Thanks,

Christoph Cullmann

Re: Taking over maintainership of Baloo

[Christoph Cullmann]

Hi,

it would be very nice to have some active maintainership there,
given a lot of VERY major crash bugs pile up there since > 1 year.

Beside that, could we get some baloo-b...@kde.org list as default assignee
for all baloo bugs instead of "one" person?

Greetings
Christoph

- Am 11. Sep 2016 um 19:08 schrieb Boudhayan Gupta bgu...@kde.org:

> Hi all,
> 
> Since Vishesh effectively stopped maintaining Baloo full-time, mostly
> Pinak and to some extent I have been looking over bugs and reviewing
> RRs pertaining to Baloo. If there are no objections to the same
> (including if Pinak declines to either co-maintain it or would rather
> maintain it alone, sans me), I'd like to officially take over
> maintainership of Baloo, co-maintaining it with Pinak.
> 
> Thanks,
> Boudhayan

-- 
- Dr.-Ing. Christoph Cullmann -
AbsInt Angewandte Informatik GmbH  Email: cullm...@absint.com
Science Park 1 Tel:   +49-681-38360-22
66123 Saarbrücken  Fax:   +49-681-38360-20
GERMANYWWW:   http://www.AbsInt.com

Geschäftsführung: Dr.-Ing. Christian Ferdinand
Eingetragen im Handelsregister des Amtsgerichts Saarbrücken, HRB 11234

Re: Review Request 128888: Improve epub extractor, less segfaults

[Boudhayan Gupta]

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


Ship it!




It looks sane enough to me. I'm going to assume you tested things and nothing 
obvious regressed, and give you the ship-it.

I'm also very interested in knowing how we have come about having a variable 
called `tit` ;-)

- Boudhayan Gupta


On Sept. 11, 2016, 10:32 p.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/12/
> ---
> 
> (Updated Sept. 11, 2016, 10:32 p.m.)
> 
> 
> Review request for KDE Frameworks, Boudhayan Gupta and Pinak Ahuja.
> 
> 
> Repository: kfilemetadata
> 
> 
> Description
> ---
> 
> Improve epub extractor:
> 
> 1) check for more nullpointers (e.g. data can be null for some fields, 
> iterators, ...)
> 2) actually close the epub file again at all
> 3) iterator seems to handle clink as stated in docs, fix double free
> 
> e.g. see bug 361727
> could be the double freed clink in the last iterator
> 
> https://bugs.kde.org/show_bug.cgi?id=361727
> 
> 
> Diffs
> -
> 
>   src/extractors/epubextractor.cpp 88a4caa 
> 
> Diff: https://git.reviewboard.kde.org/r/12/diff/
> 
> 
> Testing
> ---
> 
> Unit test still works, no idea how to craft files broken enough to trigger 
> the faults
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Taking over maintainership of Baloo

[Boudhayan Gupta]

Hi all,

Since Vishesh effectively stopped maintaining Baloo full-time, mostly
Pinak and to some extent I have been looking over bugs and reviewing
RRs pertaining to Baloo. If there are no objections to the same
(including if Pinak declines to either co-maintain it or would rather
maintain it alone, sans me), I'd like to officially take over
maintainership of Baloo, co-maintaining it with Pinak.

Thanks,
Boudhayan

Re: Review Request 128885: Increase size limit of baloo index for 64-bit machines

[Christoph Cullmann]

> On Sept. 11, 2016, 4:50 p.m., Boudhayan Gupta wrote:
> > src/engine/database.cpp, line 107
> > 
> >
> > This is clever, but I had to read it thrice to figure out what exactly 
> > it does. Maybe use this:
> > 
> > ```
> > ((sizeof(size_t) == 4) ? 1 : 256) * size_t(1024 * 1024 * 1024)
> > ```
> > 
> > Seems more readable to me - the significant digits of the size followed 
> > by the multiplier.
> > 
> > If there's a performance or strictness issue I'm not seeing, please 
> > tell me.
> 
> Boudhayan Gupta wrote:
> Err, could you also calculate the effective size in a const on the line 
> above? Sorry about the nitpick, I just want readability.

done + submitted
performance doesn't matter at all, this is done once and only arithmetics, who 
cares ;=)


- Christoph


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


On Sept. 11, 2016, 4:58 p.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128885/
> ---
> 
> (Updated Sept. 11, 2016, 4:58 p.m.)
> 
> 
> Review request for KDE Frameworks, Boudhayan Gupta, David Faure, and Pinak 
> Ahuja.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Increase size limit of baloo index for 64-bit machines to avoid crashs after 
> > 5GB of index size.
> (Better would be additional out-of-space handling, but ATM baloo has zero 
> checks for that)
> 
> The size limit for 32-bit is still 1GB, like before (there was a silent 
> overflow from 5GB to 1GB in the computation), people with large homes will 
> still get random segfaults on 32-bit.
> 
> Patch based on patch from Hao Zhang, Bug 364475
> https://bugs.kde.org/show_bug.cgi?id=364475
> 
> 
> Diffs
> -
> 
>   src/engine/database.cpp 89e2e03 
>   src/engine/databasesize.h aa180fb 
>   src/engine/transaction.cpp 0af20be 
>   src/tools/balooctl/statuscommand.cpp 73289c4 
> 
> Diff: https://git.reviewboard.kde.org/r/128885/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128885: Increase size limit of baloo index for 64-bit machines

[Christoph Cullmann]

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

(Updated Sept. 11, 2016, 4:58 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, Boudhayan Gupta, David Faure, and Pinak 
Ahuja.


Changes
---

Submitted with commit b0890aca71aa4f0fdabe65ee7b7fbd0bc844d8b8 by Christoph 
Cullmann to branch master.


Repository: baloo


Description
---

Increase size limit of baloo index for 64-bit machines to avoid crashs after > 
5GB of index size.
(Better would be additional out-of-space handling, but ATM baloo has zero 
checks for that)

The size limit for 32-bit is still 1GB, like before (there was a silent 
overflow from 5GB to 1GB in the computation), people with large homes will 
still get random segfaults on 32-bit.

Patch based on patch from Hao Zhang, Bug 364475
https://bugs.kde.org/show_bug.cgi?id=364475


Diffs
-

  src/engine/database.cpp 89e2e03 
  src/engine/databasesize.h aa180fb 
  src/engine/transaction.cpp 0af20be 
  src/tools/balooctl/statuscommand.cpp 73289c4 

Diff: https://git.reviewboard.kde.org/r/128885/diff/


Testing
---


Thanks,

Christoph Cullmann

Re: Review Request 128885: Increase size limit of baloo index for 64-bit machines

[Boudhayan Gupta]

> On Sept. 11, 2016, 10:20 p.m., Boudhayan Gupta wrote:
> > src/engine/database.cpp, line 107
> > 
> >
> > This is clever, but I had to read it thrice to figure out what exactly 
> > it does. Maybe use this:
> > 
> > ```
> > ((sizeof(size_t) == 4) ? 1 : 256) * size_t(1024 * 1024 * 1024)
> > ```
> > 
> > Seems more readable to me - the significant digits of the size followed 
> > by the multiplier.
> > 
> > If there's a performance or strictness issue I'm not seeing, please 
> > tell me.

Err, could you also calculate the effective size in a const on the line above? 
Sorry about the nitpick, I just want readability.


- Boudhayan


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


On Sept. 11, 2016, 4:27 p.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128885/
> ---
> 
> (Updated Sept. 11, 2016, 4:27 p.m.)
> 
> 
> Review request for KDE Frameworks, Boudhayan Gupta, David Faure, and Pinak 
> Ahuja.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Increase size limit of baloo index for 64-bit machines to avoid crashs after 
> > 5GB of index size.
> (Better would be additional out-of-space handling, but ATM baloo has zero 
> checks for that)
> 
> The size limit for 32-bit is still 1GB, like before (there was a silent 
> overflow from 5GB to 1GB in the computation), people with large homes will 
> still get random segfaults on 32-bit.
> 
> Patch based on patch from Hao Zhang, Bug 364475
> https://bugs.kde.org/show_bug.cgi?id=364475
> 
> 
> Diffs
> -
> 
>   src/engine/database.cpp 89e2e03 
>   src/engine/databasesize.h aa180fb 
>   src/engine/transaction.cpp 0af20be 
>   src/tools/balooctl/statuscommand.cpp 73289c4 
> 
> Diff: https://git.reviewboard.kde.org/r/128885/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128887: ctime/mtime => 0 (which is perfectly OK) => baloo dead

[Christoph Cullmann]

> On Sept. 11, 2016, 3:05 p.m., Sune Vuorela wrote:
> > Looks good to me. I would though appreciate a unit test if ever possible.  
> > It would be an easy bug to reintroduce.
> 
> Christoph Cullmann wrote:
> I can try to create one ;=)

Added unit test + pushed


- Christoph


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


On Sept. 11, 2016, 4:52 p.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128887/
> ---
> 
> (Updated Sept. 11, 2016, 4:52 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Fix that baloo is instant killed by any file with timestamp 0. (which is OK 
> and can easily happen after unpacking some zip/tar/..)
> 
> Bug 355238
> https://bugs.kde.org/show_bug.cgi?id=355238
> 
> 
> Diffs
> -
> 
>   src/engine/documenttimedb.cpp aa0925a 
>   src/engine/writetransaction.cpp 9ad7520 
> 
> Diff: https://git.reviewboard.kde.org/r/128887/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128887: ctime/mtime => 0 (which is perfectly OK) => baloo dead

[Christoph Cullmann]

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

(Updated Sept. 11, 2016, 6:52 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Changes
---

Submitted with commit 628daced19b88d0c537736a14aea3287a4662609 by Christoph 
Cullmann to branch master.


Repository: baloo


Description
---

Fix that baloo is instant killed by any file with timestamp 0. (which is OK and 
can easily happen after unpacking some zip/tar/..)

Bug 355238
https://bugs.kde.org/show_bug.cgi?id=355238


Diffs
-

  src/engine/documenttimedb.cpp aa0925a 
  src/engine/writetransaction.cpp 9ad7520 

Diff: https://git.reviewboard.kde.org/r/128887/diff/


Testing
---


Thanks,

Christoph Cullmann

Re: Review Request 128885: Increase size limit of baloo index for 64-bit machines

[Boudhayan Gupta]

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


Fix it, then Ship it!




Ship It!


src/engine/database.cpp (line 106)


This is clever, but I had to read it thrice to figure out what exactly it 
does. Maybe use this:

```
((sizeof(size_t) == 4) ? 1 : 256) * size_t(1024 * 1024 * 1024)
```

Seems more readable to me - the significant digits of the size followed by 
the multiplier.

If there's a performance or strictness issue I'm not seeing, please tell me.


- Boudhayan Gupta


On Sept. 11, 2016, 4:27 p.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128885/
> ---
> 
> (Updated Sept. 11, 2016, 4:27 p.m.)
> 
> 
> Review request for KDE Frameworks, Boudhayan Gupta, David Faure, and Pinak 
> Ahuja.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Increase size limit of baloo index for 64-bit machines to avoid crashs after 
> > 5GB of index size.
> (Better would be additional out-of-space handling, but ATM baloo has zero 
> checks for that)
> 
> The size limit for 32-bit is still 1GB, like before (there was a silent 
> overflow from 5GB to 1GB in the computation), people with large homes will 
> still get random segfaults on 32-bit.
> 
> Patch based on patch from Hao Zhang, Bug 364475
> https://bugs.kde.org/show_bug.cgi?id=364475
> 
> 
> Diffs
> -
> 
>   src/engine/database.cpp 89e2e03 
>   src/engine/databasesize.h aa180fb 
>   src/engine/transaction.cpp 0af20be 
>   src/tools/balooctl/statuscommand.cpp 73289c4 
> 
> Diff: https://git.reviewboard.kde.org/r/128885/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 120000: New Thermal and Electrical Units and Unit Convienience Function for KUnitConversion

[Garret Wassermann]

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

(Updated Sept. 11, 2016, 12:17 p.m.)


Review request for KDE Frameworks, Garret Wassermann, John Layt, and Petri 
Damstén.


Changes
---

Updated to @since 5.27. I saw that 5.26 was released yesterday without this 
patch.


Repository: kunitconversion


Description
---

New Thermal and Electrical Units and Convienience Function for KUnitConversion


Diffs (updated)
-

  README.md fe8cbe8 
  src/CMakeLists.txt ac4a92a 
  src/converter.cpp 0a7f862 
  src/electrical_current.cpp PRE-CREATION 
  src/electrical_current_p.h PRE-CREATION 
  src/electrical_resistance.cpp PRE-CREATION 
  src/electrical_resistance_p.h PRE-CREATION 
  src/energy.cpp f525031 
  src/thermal_conductivity.cpp PRE-CREATION 
  src/thermal_conductivity_p.h PRE-CREATION 
  src/thermal_flux.cpp PRE-CREATION 
  src/thermal_flux_p.h PRE-CREATION 
  src/thermal_generation.cpp PRE-CREATION 
  src/thermal_generation_p.h PRE-CREATION 
  src/unit.h e440346 
  src/voltage.cpp PRE-CREATION 
  src/voltage_p.h PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/12/diff/


Testing
---

I added some fundamental (base SI) units as well as some common derived units 
in SI and English/Imperial system, mostly for electrical or thermal units. I 
also added a simple member function to the Value class, symbolAsString(), to 
immediately receive the current unit assigned to the Value object as a QString. 
I was running into issues with my code where doing v.unit()->symbol() wasn't 
working because I was getting KSharedPointers instead of the object, and the 
program would just crash. I just wanted the string! So I made a function, which 
helped with testing of the units.

The new categories work fine under my test program. I was able to convert 
thermal units back and forth in my test program, so seems ok. The values also 
appear correct from my tests (as long as I didn't mess anything up by hand!).

First time submitting review, so please let me know if there is something else 
I should do better next time, thank you.


Thanks,

Garret Wassermann

Re: Review Request 128889: Fix KFileItem::overlays returning empty string values

[Chinmoy Ranjan Pradhan]

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

(Updated Sept. 11, 2016, 4:16 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and David Faure.


Repository: kio


Description
---

KFileItem::overlays always returns a string list with an empty string as the 
list's first element.
For example, 
for a symlink the list will be like this: {"", emblem-symbiolic-link}
and for a plain text file it'll be like : {""} (here it should be completely 
empty, size=0)
This patch attempts to fix the issue.


Diffs
-

  src/core/kfileitem.cpp b5ed5fd 

Diff: https://git.reviewboard.kde.org/r/128889/diff/


Testing
---


Thanks,

Chinmoy Ranjan Pradhan

Re: Review Request 128887: ctime/mtime => 0 (which is perfectly OK) => baloo dead

[Christoph Cullmann]

> On Sept. 11, 2016, 3:05 p.m., Sune Vuorela wrote:
> > Looks good to me. I would though appreciate a unit test if ever possible.  
> > It would be an easy bug to reintroduce.

I can try to create one ;=)


- Christoph


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


On Sept. 11, 2016, 11:49 a.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128887/
> ---
> 
> (Updated Sept. 11, 2016, 11:49 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Fix that baloo is instant killed by any file with timestamp 0. (which is OK 
> and can easily happen after unpacking some zip/tar/..)
> 
> Bug 355238
> https://bugs.kde.org/show_bug.cgi?id=355238
> 
> 
> Diffs
> -
> 
>   src/engine/documenttimedb.cpp aa0925a 
>   src/engine/writetransaction.cpp 9ad7520 
> 
> Diff: https://git.reviewboard.kde.org/r/128887/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128889: Fix KFileItem::overlays returning empty string values

[David Faure]

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


Ship it!




Ship It!

- David Faure


On sep. 11, 2016, 3:40 après-midi, Chinmoy Ranjan Pradhan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128889/
> ---
> 
> (Updated sep. 11, 2016, 3:40 après-midi)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> KFileItem::overlays always returns a string list with an empty string as the 
> list's first element.
> For example, 
> for a symlink the list will be like this: {"", emblem-symbiolic-link}
> and for a plain text file it'll be like : {""} (here it should be completely 
> empty, size=0)
> This patch attempts to fix the issue.
> 
> 
> Diffs
> -
> 
>   src/core/kfileitem.cpp b5ed5fd 
> 
> Diff: https://git.reviewboard.kde.org/r/128889/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Chinmoy Ranjan Pradhan
> 
>

Review Request 128889: Fix KFileItem::overlays returning empty string values

[Chinmoy Ranjan Pradhan]

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

Review request for KDE Frameworks and David Faure.


Repository: kio


Description
---

KFileItem::overlays always returns a string list with an empty string as the 
list's first element.
For example, 
for a symlink the list will be like this: {"", emblem-symbiolic-link}
and for a plain text file it'll be like : {""} (here it should be completely 
empty, size=0)
This patch attempts to fix the issue.


Diffs
-

  src/core/kfileitem.cpp b5ed5fd 

Diff: https://git.reviewboard.kde.org/r/128889/diff/


Testing
---


Thanks,

Chinmoy Ranjan Pradhan

Re: Review Request 128887: ctime/mtime => 0 (which is perfectly OK) => baloo dead

[Sune Vuorela]

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



Looks good to me. I would though appreciate a unit test if ever possible.  It 
would be an easy bug to reintroduce.

- Sune Vuorela


On Sept. 11, 2016, 11:49 a.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128887/
> ---
> 
> (Updated Sept. 11, 2016, 11:49 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Fix that baloo is instant killed by any file with timestamp 0. (which is OK 
> and can easily happen after unpacking some zip/tar/..)
> 
> Bug 355238
> https://bugs.kde.org/show_bug.cgi?id=355238
> 
> 
> Diffs
> -
> 
>   src/engine/documenttimedb.cpp aa0925a 
>   src/engine/writetransaction.cpp 9ad7520 
> 
> Diff: https://git.reviewboard.kde.org/r/128887/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128885: Increase size limit of baloo index for 64-bit machines

[Sune Vuorela]

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



Just checked that the engine.h is not installed, so no problems in changing the 
data types from a binary compatibility point of view.

- Sune Vuorela


On Sept. 11, 2016, 10:57 a.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128885/
> ---
> 
> (Updated Sept. 11, 2016, 10:57 a.m.)
> 
> 
> Review request for KDE Frameworks, Boudhayan Gupta, David Faure, and Pinak 
> Ahuja.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Increase size limit of baloo index for 64-bit machines to avoid crashs after 
> > 5GB of index size.
> (Better would be additional out-of-space handling, but ATM baloo has zero 
> checks for that)
> 
> The size limit for 32-bit is still 1GB, like before (there was a silent 
> overflow from 5GB to 1GB in the computation), people with large homes will 
> still get random segfaults on 32-bit.
> 
> Patch based on patch from Hao Zhang, Bug 364475
> https://bugs.kde.org/show_bug.cgi?id=364475
> 
> 
> Diffs
> -
> 
>   src/engine/database.cpp 89e2e03 
>   src/engine/databasesize.h aa180fb 
>   src/engine/transaction.cpp 0af20be 
>   src/tools/balooctl/statuscommand.cpp 73289c4 
> 
> Diff: https://git.reviewboard.kde.org/r/128885/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128886: odf files with no content.xml crash indexer

[Christoph Cullmann]

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

(Updated Sept. 11, 2016, 1:08 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, Boudhayan Gupta, David Faure, and Pinak 
Ahuja.


Changes
---

Submitted with commit 40730d75397aefb92145f86fc6abc9b303c56cfe by Christoph 
Cullmann to branch master.


Repository: kfilemetadata


Description
---

Make odf indexer more error prove, check if the files are there (and are files 
at all) (meta.xml + content.xml)


Diffs
-

  autotests/odfextractortest.h 3e6a1c7 
  autotests/odfextractortest.cpp bea3640 
  src/extractors/odfextractor.cpp 948f3e0 

Diff: https://git.reviewboard.kde.org/r/128886/diff/


Testing
---

Added unit tests, before, the missing content.xml did directly segfault baloo 
and co.

See Bug 364748
https://bugs.kde.org/show_bug.cgi?id=364748


File Attachments


missing content test
  
https://git.reviewboard.kde.org/media/uploaded/files/2016/09/11/12206883-a130-4335-bb35-7514d832dc81__test_missing_content.odt
missing meta test
  
https://git.reviewboard.kde.org/media/uploaded/files/2016/09/11/eb7bdd4b-f2d5-4501-bd88-9ae492bb67d8__test_missing_meta.odt


Thanks,

Christoph Cullmann

Review Request 128888: Improve epub extractor, less segfaults

[Christoph Cullmann]

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

Review request for KDE Frameworks and Pinak Ahuja.


Repository: kfilemetadata


Description
---

Improve epub extractor:

1) check for more nullpointers (e.g. data can be null for some fields, 
iterators, ...)
2) actually close the epub file again at all
3) iterator seems to handle clink as stated in docs, fix double free

e.g. see bug 361727
could be the double freed clink in the last iterator

https://bugs.kde.org/show_bug.cgi?id=361727


Diffs
-

  src/extractors/epubextractor.cpp 88a4caa 

Diff: https://git.reviewboard.kde.org/r/12/diff/


Testing
---

Unit test still works, no idea how to craft files broken enough to trigger the 
faults


Thanks,

Christoph Cullmann

Review Request 128887: ctime/mtime => 0 (which is perfectly OK) => baloo dead

[Christoph Cullmann]

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

Review request for KDE Frameworks.


Repository: baloo


Description
---

Fix that baloo is instant killed by any file with timestamp 0. (which is OK and 
can easily happen after unpacking some zip/tar/..)

Bug 355238
https://bugs.kde.org/show_bug.cgi?id=355238


Diffs
-

  src/engine/documenttimedb.cpp aa0925a 
  src/engine/writetransaction.cpp 9ad7520 

Diff: https://git.reviewboard.kde.org/r/128887/diff/


Testing
---


Thanks,

Christoph Cullmann

Re: Review Request 128886: odf files with no content.xml crash indexer

[Christoph Cullmann]

> On Sept. 11, 2016, 11:22 a.m., Kai Uwe Broulik wrote:
> > autotests/odfextractortest.cpp, line 103
> > 
> >
> > Either set a parent or use QScopedPointer?

Can change that, just copied the boiler plate from the other tests ;=) (will 
change it there, then, too)


- Christoph


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


On Sept. 11, 2016, 11:18 a.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128886/
> ---
> 
> (Updated Sept. 11, 2016, 11:18 a.m.)
> 
> 
> Review request for KDE Frameworks, Boudhayan Gupta, David Faure, and Pinak 
> Ahuja.
> 
> 
> Repository: kfilemetadata
> 
> 
> Description
> ---
> 
> Make odf indexer more error prove, check if the files are there (and are 
> files at all) (meta.xml + content.xml)
> 
> 
> Diffs
> -
> 
>   autotests/odfextractortest.h 3e6a1c7 
>   autotests/odfextractortest.cpp bea3640 
>   src/extractors/odfextractor.cpp 948f3e0 
> 
> Diff: https://git.reviewboard.kde.org/r/128886/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests, before, the missing content.xml did directly segfault baloo 
> and co.
> 
> See Bug 364748
> https://bugs.kde.org/show_bug.cgi?id=364748
> 
> 
> File Attachments
> 
> 
> missing content test
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/09/11/12206883-a130-4335-bb35-7514d832dc81__test_missing_content.odt
> missing meta test
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/09/11/eb7bdd4b-f2d5-4501-bd88-9ae492bb67d8__test_missing_meta.odt
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128886: odf files with no content.xml crash indexer

[Kai Uwe Broulik]

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



lgtm


autotests/odfextractortest.cpp (line 103)


Either set a parent or use QScopedPointer?


- Kai Uwe Broulik


On Sept. 11, 2016, 11:18 vorm., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128886/
> ---
> 
> (Updated Sept. 11, 2016, 11:18 vorm.)
> 
> 
> Review request for KDE Frameworks, Boudhayan Gupta, David Faure, and Pinak 
> Ahuja.
> 
> 
> Repository: kfilemetadata
> 
> 
> Description
> ---
> 
> Make odf indexer more error prove, check if the files are there (and are 
> files at all) (meta.xml + content.xml)
> 
> 
> Diffs
> -
> 
>   autotests/odfextractortest.h 3e6a1c7 
>   autotests/odfextractortest.cpp bea3640 
>   src/extractors/odfextractor.cpp 948f3e0 
> 
> Diff: https://git.reviewboard.kde.org/r/128886/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests, before, the missing content.xml did directly segfault baloo 
> and co.
> 
> See Bug 364748
> https://bugs.kde.org/show_bug.cgi?id=364748
> 
> 
> File Attachments
> 
> 
> missing content test
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/09/11/12206883-a130-4335-bb35-7514d832dc81__test_missing_content.odt
> missing meta test
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/09/11/eb7bdd4b-f2d5-4501-bd88-9ae492bb67d8__test_missing_meta.odt
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128886: odf files with no content.xml crash indexer

[Christoph Cullmann]

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

(Updated Sept. 11, 2016, 11:18 a.m.)


Review request for KDE Frameworks, Boudhayan Gupta, David Faure, and Pinak 
Ahuja.


Repository: kfilemetadata


Description
---

Make odf indexer more error prove, check if the files are there (and are files 
at all) (meta.xml + content.xml)


Diffs
-

  autotests/odfextractortest.h 3e6a1c7 
  autotests/odfextractortest.cpp bea3640 
  src/extractors/odfextractor.cpp 948f3e0 

Diff: https://git.reviewboard.kde.org/r/128886/diff/


Testing
---

Added unit tests, before, the missing content.xml did directly segfault baloo 
and co.

See Bug 364748
https://bugs.kde.org/show_bug.cgi?id=364748


File Attachments (updated)


missing content test
  
https://git.reviewboard.kde.org/media/uploaded/files/2016/09/11/12206883-a130-4335-bb35-7514d832dc81__test_missing_content.odt
missing meta test
  
https://git.reviewboard.kde.org/media/uploaded/files/2016/09/11/eb7bdd4b-f2d5-4501-bd88-9ae492bb67d8__test_missing_meta.odt


Thanks,

Christoph Cullmann

Review Request 128886: odf files with no content.xml crash indexer

[Christoph Cullmann]

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

Review request for KDE Frameworks, Boudhayan Gupta, David Faure, and Pinak 
Ahuja.


Repository: kfilemetadata


Description
---

Make odf indexer more error prove, check if the files are there (and are files 
at all) (meta.xml + content.xml)


Diffs
-

  autotests/odfextractortest.h 3e6a1c7 
  autotests/odfextractortest.cpp bea3640 
  src/extractors/odfextractor.cpp 948f3e0 

Diff: https://git.reviewboard.kde.org/r/128886/diff/


Testing
---

Added unit tests, before, the missing content.xml did directly segfault baloo 
and co.

See Bug 364748
https://bugs.kde.org/show_bug.cgi?id=364748


Thanks,

Christoph Cullmann

Review Request 128885: Increase size limit of baloo index for 64-bit machines

[Christoph Cullmann]

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

Review request for KDE Frameworks, Boudhayan Gupta, David Faure, and Pinak 
Ahuja.


Repository: baloo


Description
---

Increase size limit of baloo index for 64-bit machines to avoid crashs after > 
5GB of index size.
(Better would be additional out-of-space handling, but ATM baloo has zero 
checks for that)

The size limit for 32-bit is still 1GB, like before (there was a silent 
overflow from 5GB to 1GB in the computation), people with large homes will 
still get random segfaults on 32-bit.

Patch based on patch from Hao Zhang, Bug 364475
https://bugs.kde.org/show_bug.cgi?id=364475


Diffs
-

  src/engine/database.cpp 89e2e03 
  src/engine/databasesize.h aa180fb 
  src/engine/transaction.cpp 0af20be 
  src/tools/balooctl/statuscommand.cpp 73289c4 

Diff: https://git.reviewboard.kde.org/r/128885/diff/


Testing
---


Thanks,

Christoph Cullmann

Re: Review Request/New Framework: KF5::SyntaxHighlighting

[Volker Krause]

On Sunday 11 September 2016 05:33:29 Michael Palimaka wrote:
> On 11/09/16 01:47, Volker Krause wrote:
> > please review KF5::SyntaxHighlighting (syntax-highlighting in Git) for
> > becoming a framework :)
> 
> Thanks a lot for working on this.
> 
> I noticed that KF5SyntaxHighlightingConfig.cmake.in searches for
> Qt5Widgets, but that doesn't seem to used in any public part of the
> framework (only tests). 

indeed, fixed.


> Also, some tests are failing here.

did you maybe download updated syntax definitions, or have those installed via 
Kate already? If so, that indeed broke the tests, that should be fixed now 
though. If the tests still fail for you, please send me some more details, in 
particular the full output.

Thanks!
Volker

signature.asc
Description: This is a digitally signed message part.