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
