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

Reply via email to