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
