D11236: [KCrash] Establish socket to allow change of ptracer

[Oswald Buddenhagen]

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


  mostly minor issues left.
  
  please explicitly mark all handled issues as done - you'll notice on the way that you didn't address some of them.
  
  you adjusted the summary kinda the wrong way around ... but come to think of it, i was actually kinda wrong - you indeed need to list all three cases to illustrate that neither applies. the ancestry case is special only in the sense that it automatically makes the "internal" debugger work (i'd mention that in parentheses of that case's bullet point).

INLINE COMMENTS

> kcrash.cpp:668
> +            QStringLiteral("%1/kcrash_%2").arg(QStandardPaths::writableLocation(QStandardPaths::RuntimeLocation))
> +                                          .arg(pid));
> +        int sockfd = openDrKonqiSocket(socketpath);

not really significant, but imo it's backwards that you let the drkonqi pid rather than the kcrash pid determine the socket name.

> kcrash.cpp:674
> +            if (directly) {
> +                //if the process was started directly, use waitpid(), as it's a child...
> +                while ((running = waitpid(pid, nullptr, WNOHANG) != pid) && pollDrKonqiSocket(pid, sockfd) >= 0) {}

please consistently put a space after the // marker. also, stick to the file's capitalization style.

> kcrash.cpp:686
> +        // Wait forever until the started process exits. This code path is executed
> +        // when launching drkonqi. Note that drkonqi will SIGSTOP this process in the meantime.
> +        if (running) {

hmm, what about the last sentence? it seems to me that some adjustment (possibly just additional comments) is required.

> kcrash.cpp:870
> +{
> +    struct sockaddr_un drkonqi_server;
> +

i'd move that down, before the member init.

REPOSITORY
  R285 KCrash

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

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190116/c6a674f7/attachment-0001.html>