D29805: Thumbnail djvu: avoid to exit(1) when it should not

[Pino Toscano]

pino requested changes to this revision.
pino added a comment.
This revision now requires changes to proceed.


  This change is definitely not correct.
  
  > A child process exited with 1 as result but it wasn't an error case.
  
  it is an error case: the execv*() family [1] of functions replaces the execution with the requested process, and //never return// in a successful execution. "successful execution" means that the process was started correctly, no matter its resulting exit code. Anything after execv*() will be processed only if it fails, and one of the most common issues is that the requested executable does not exists.
  
  In addition to this, your case merely changes the exit() code, still calling exit() (which appears in the backtraces of the two bugs, and most probably one can be marked as duplicate of the other). The issue here is that fork() //clones// the calling process (in this case kdeinit), including the atexit handlers: apparently a mesa atexit handler is run... The real fix in this case is to use _exit() [2], which is a more direct way to exit the current process without running the atexit handlers.
  
  Furthermore, this code looks like a C snippet to invoke a process and reading its stdout retrofitted as ThumbCreator plugin... sadly C is not an easy language, and the process APIs are complex and very prone to mistakes. The ideal solution would be to invoke ddjvu using QProcess, and read its output as it comes; it would also have the (small) advantage of making this thumbnailer plugin available on any platform, Windows included (in case anyone cases, that is).
  
  Let me sum up what I think is the approach to make this ThumbCreator work properly:
  
  - to fix the issue in the stable branch:
    - switch exit() wth _exit(), using the same exit code (otherwise `ok` won't be set as false after the waitpid() call in the parent)
    - before the pipe creation, check for the ddjvu executable existance using QStandardPath::findExecutable, returning false quickly
  - to fix the issue in the long term:
    - rewrite this to call ddjvu using QProcess
  
  [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html
  [2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/_Exit.html

REPOSITORY
  R320 KIO Extras

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

To: meven, #frameworks, broulik, ngraham, pino
Cc: pino, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20200517/25bbfb09/attachment.htm>