D16523: [Extractor] Replace homegrown IO handler with QDataStream, catch HUP

2018-11-05 Thread Stefan Brüns 
This revision was automatically updated to reflect the committed changes.
Closed by commit R293:52107a8af3ee: [Extractor] Replace homegrown IO handler 
with QDataStream, catch HUP (authored by bruns).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16523?vs=44476=44939

REVISION DETAIL
  https://phabricator.kde.org/D16523

AFFECTED FILES
  src/file/extractor/CMakeLists.txt
  src/file/extractor/app.cpp
  src/file/extractor/app.h
  src/file/extractor/autotests/CMakeLists.txt
  src/file/extractor/autotests/iohandlertest.cpp
  src/file/extractor/iohandler.cpp
  src/file/extractor/iohandler.h

To: bruns, #baloo, #frameworks, ngraham, poboiko, lbeltrame
Cc: lbeltrame, kde-frameworks-devel, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D16523: [Extractor] Replace homegrown IO handler with QDataStream, catch HUP

2018-11-05 Thread Luca Beltrame 
lbeltrame accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R293 Baloo

BRANCH
  extractor

REVISION DETAIL
  https://phabricator.kde.org/D16523

To: bruns, #baloo, #frameworks, ngraham, poboiko, lbeltrame
Cc: lbeltrame, kde-frameworks-devel, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D16523: [Extractor] Replace homegrown IO handler with QDataStream, catch HUP

2018-11-05 Thread Stefan Brüns 
bruns added a comment.


  good to go?

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D16523

To: bruns, #baloo, #frameworks, ngraham, poboiko
Cc: lbeltrame, kde-frameworks-devel, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D16523: [Extractor] Replace homegrown IO handler with QDataStream, catch HUP

2018-11-02 Thread Stefan Brüns 
bruns marked an inline comment as done.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D16523

To: bruns, #baloo, #frameworks, ngraham, poboiko
Cc: lbeltrame, kde-frameworks-devel, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D16523: [Extractor] Replace homegrown IO handler with QDataStream, catch HUP

2018-10-30 Thread Stefan Brüns 
bruns marked an inline comment as done.
bruns added inline comments.

INLINE COMMENTS

> poboiko wrote in app.cpp:74
> I haven't work with `QDataStream` before, but shouldn't `m_inputStream >> 
> nIds` do the same? (and same below)
> And probably you should avoid magical constants (4, 8, etc) - better use 
> corresponding `sizeof`s

readRawData consumes N bytes, while operator>> consumes N + 
sizeof(serialization meta data).

I don't care to much for this code, as it is removed in the followup patch.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D16523

To: bruns, #baloo, #frameworks, ngraham, poboiko
Cc: lbeltrame, kde-frameworks-devel, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D16523: [Extractor] Replace homegrown IO handler with QDataStream, catch HUP

2018-10-30 Thread Stefan Brüns 
bruns added a comment.


  In D16523#350888 , @poboiko wrote:
  
  > That's nice! I'll test it a little.
  
  
  This step keeps binary compatibility with the current streaming format. The 
big cleanup happens in the next commit (D16524 
),

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D16523

To: bruns, #baloo, #frameworks, ngraham, poboiko
Cc: lbeltrame, kde-frameworks-devel, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D16523: [Extractor] Replace homegrown IO handler with QDataStream, catch HUP

2018-10-30 Thread Igor Poboiko 
poboiko added a comment.


  That's nice! I'll test it a little.

INLINE COMMENTS

> app.cpp:74
> +
> +memcpy(, buf, 4);
> +for (quint32 i = 0; i < nIds; i++) {

I haven't work with `QDataStream` before, but shouldn't `m_inputStream >> nIds` 
do the same? (and same below)
And probably you should avoid magical constants (4, 8, etc) - better use 
corresponding `sizeof`s

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D16523

To: bruns, #baloo, #frameworks, ngraham, poboiko
Cc: lbeltrame, kde-frameworks-devel, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D16523: [Extractor] Replace homegrown IO handler with QDataStream, catch HUP

2018-10-30 Thread Luca Beltrame 
lbeltrame added a comment.


  In general (for my limited Baloo knowledge) this makes sense. You might want 
to add a few CCBUGs if you are aware of specific bugs this alleviates (@ngraham 
or someone from the bugsquad may help).

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D16523

To: bruns, #baloo, #frameworks, ngraham, poboiko
Cc: lbeltrame, kde-frameworks-devel, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D16523: [Extractor] Replace homegrown IO handler with QDataStream, catch HUP

2018-10-29 Thread Stefan Brüns 
bruns added a dependent revision: D16524: [Extractor] Use QDataStream 
serialization in place of cooked one.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D16523

To: bruns, #baloo, #frameworks, ngraham, poboiko
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D16523: [Extractor] Replace homegrown IO handler with QDataStream, catch HUP

2018-10-29 Thread Stefan Brüns 
bruns created this revision.
bruns added reviewers: Baloo, Frameworks, ngraham, poboiko.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  The handler does not handle partial reads, and most importantly, does
  not handle when the pipe from the parent process is closed.
  
  Although this should not happen during normal operation, in case of a
  crash the indexer process will receive QSocketNotifier::activated events
  due to 'POLLHUP' events from the underlying poll. This causes a busy
  loop, as the underlying pipe status is never checked.
  
  May fix a few instances of "100% CPU load" bug reports.

REPOSITORY
  R293 Baloo

BRANCH
  extractor

REVISION DETAIL
  https://phabricator.kde.org/D16523

AFFECTED FILES
  src/file/extractor/CMakeLists.txt
  src/file/extractor/app.cpp
  src/file/extractor/app.h
  src/file/extractor/autotests/CMakeLists.txt
  src/file/extractor/autotests/iohandlertest.cpp
  src/file/extractor/iohandler.cpp
  src/file/extractor/iohandler.h

To: bruns, #baloo, #frameworks, ngraham, poboiko
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams