ossi requested changes to this revision.
ossi added a comment.
This revision now requires changes to proceed.
yay, i finally have "some" time for this. ^^
it took me a while to get a coherent picture of the problem and solution,
because your description unnecessarily dwells on the irrelevant cases of ptrace
restrictions, but omits the crucial fact that this is about tracers that are
not descendant processes of drkonqi, specifically kdevelop launched via
kdeinit. see also comment on DefaultDebuggerLauncher in the complementary
change. please make sure to point that out in the commit messages.
the socket code seemed like an unnecessary complication at first, but then i
realized that launching via kdeinit makes it impossible to just talk to the
child via stdout/stderr or at least pass a file descriptor of a socketpair end.
that's something that could be actually addressed in the klauncher api, because
it's possible to pass open fds to other processes over sockets.
INLINE COMMENTS
> kcrash.cpp:655
> //if the process was started directly, use waitpid(), as it's a
> child...
> while (waitpid(-1, nullptr, 0) != pid) {}
> } else {
you need to cover that branch as well.
> kcrash.cpp:668
> + // create socket path to transfer ptrace scope and open
> connection
> + const QByteArray socketpath =
> QStringLiteral("%1/kcrash_%2").arg(QDir::tempPath()).arg(pid).toLocal8Bit();
> + int sockfd = openDrKonqiSocket(socketpath);
use RuntimeLocation, as the kdeinit-related code (at start of
setCrashHandler()) does.
> kcrash.cpp:870
> +
> + if (bind(sockfd, (struct sockaddr *)&drkonqi_server,
> sizeof(drkonqi_server)) < 0) {
> + perror("Warning: bind() for communication with DrKonqi failed");
you need to unlink first, otherwise stale sockets will throw you off.
> kcrash.cpp:884
> +{
> + fd_set set;
> + FD_ZERO(&set);
why don't you use poll()? this code is linux-specific anyway, so you can rely
on posix functions from the previous decade.
> kcrash.cpp:904
> + clsockfd = accept(sockfd, (struct sockaddr *)&drkonqi_client,
> &cllength);
> + } while (clsockfd == -1 && (errno == EINTR || errno == EAGAIN));
> + if (clsockfd < 0)
EAGAIN is a workaround for some unspecified broken non-linux systems, so you
can drop that here.
> kcrash.cpp:915
> + if (ucred.pid != pid) {
> + perror("Warning: peer pid does not match DrKonqi pid");
> + return -1;
that's bogus use of perror()
REPOSITORY
R285 KCrash
REVISION DETAIL
https://phabricator.kde.org/D11236
To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh,
ngraham, bruns
