DevHeads.net

Review Request 128909: initial, minimal support for OS X

Review request for KDE Base Apps and KDE Software on Mac OS X.

Repository: libksysguard

Description
This patch implements initial and rather minimal support for OS X, for now focussing purely on process information. That feature is justified as it is used by KDevelop in order to obtain the list of processes one can attach a debugger to.

Mac OS X is tricky because it requires special privileges in order to obtain certain types of information for any running process. For example, even obtaining the number of threads spawned by a foreign process requires privileges that aren't trivial to set up. I've prepared the terrain, but also implemented a fallback strategy that calls `ps` to be sure that crucial information like the command name is available for all processes.

Diffs
processcore/CMakeLists.txt e7c9263
processcore/Info.plist PRE-CREATION
processcore/helper.cpp d54c8e1
processcore/processes_darwin_p.cpp PRE-CREATION
processcore/processes_local_p.cpp 2bc123f

Diff: <a href="https://git.reviewboard.kde.org/r/128909/diff/" title="https://git.reviewboard.kde.org/r/128909/diff/">https://git.reviewboard.kde.org/r/128909/diff/</a>

Testing
On OS X 10.9.5 with Frameworks 5.24.0 and Qt 5.6.1

Thanks,

René J.V. Bertin

Comments

Re: Review Request 128909: initial, minimal support for OS X

By =?utf-8?Q?Ren=C... at 09/14/2016 - 15:37

(Updated Sept. 14, 2016, 9:37 p.m.)

Review request for KDE Base Apps and KDE Software on Mac OS X.

Changes
inline issue hopefully fixed

Repository: libksysguard

Description
This patch implements initial and rather minimal support for OS X, for now focussing purely on process information. That feature is justified as it is used by KDevelop in order to obtain the list of processes one can attach a debugger to.

Mac OS X is tricky because it requires special privileges in order to obtain certain types of information for any running process. For example, even obtaining the number of threads spawned by a foreign process requires privileges that aren't trivial to set up. I've prepared the terrain, but also implemented a fallback strategy that calls `ps` to be sure that crucial information like the command name is available for all processes.

Diffs (updated)
processcore/CMakeLists.txt e7c9263
processcore/Info.plist PRE-CREATION
processcore/helper.cpp d54c8e1
processcore/processes_darwin_p.cpp PRE-CREATION
processcore/processes_local_p.cpp 2bc123f

Diff: <a href="https://git.reviewboard.kde.org/r/128909/diff/" title="https://git.reviewboard.kde.org/r/128909/diff/">https://git.reviewboard.kde.org/r/128909/diff/</a>

Testing
On OS X 10.9.5 with Frameworks 5.24.0 and Qt 5.6.1

File Attachments
Attach to Process widget in KDevelop
<a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png" title="https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png">https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811...</a>

Thanks,

René J.V. Bertin

Re: Review Request 128909: initial, minimal support for OS X

By =?utf-8?Q?Ren=C... at 09/15/2016 - 08:47

(Updated Sept. 15, 2016, 2:47 p.m.)

Review request for KDE Base Apps and KDE Software on Mac OS X.

Changes
drops the support for running setuid root from helper.cpp

Repository: libksysguard

Description
This patch implements initial and rather minimal support for OS X, for now focussing purely on process information. That feature is justified as it is used by KDevelop in order to obtain the list of processes one can attach a debugger to.

Mac OS X is tricky because it requires special privileges in order to obtain certain types of information for any running process. For example, even obtaining the number of threads spawned by a foreign process requires privileges that aren't trivial to set up. I've prepared the terrain, but also implemented a fallback strategy that calls `ps` to be sure that crucial information like the command name is available for all processes.

Diffs (updated)
processcore/CMakeLists.txt e7c9263
processcore/Info.plist PRE-CREATION
processcore/processes_darwin_p.cpp PRE-CREATION
processcore/processes_local_p.cpp 2bc123f

Diff: <a href="https://git.reviewboard.kde.org/r/128909/diff/" title="https://git.reviewboard.kde.org/r/128909/diff/">https://git.reviewboard.kde.org/r/128909/diff/</a>

Testing
On OS X 10.9.5 with Frameworks 5.24.0 and Qt 5.6.1

File Attachments
Attach to Process widget in KDevelop
<a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png" title="https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png">https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811...</a>

Thanks,

René J.V. Bertin

Re: Review Request 128909: initial, minimal support for OS X

By =?utf-8?Q?Ren=C... at 09/17/2016 - 10:38

(Updated Sept. 17, 2016, 4:38 p.m.)

Review request for KDE Base Apps and KDE Software on Mac OS X.

Changes
New version that disables making changes to the process parameters that cannot currently be changed. I should still do the same for sending signals to other users' processes.

Repository: libksysguard

Description
This patch implements initial and rather minimal support for OS X, for now focussing purely on process information. That feature is justified as it is used by KDevelop in order to obtain the list of processes one can attach a debugger to.

Mac OS X is tricky because it requires special privileges in order to obtain certain types of information for any running process. For example, even obtaining the number of threads spawned by a foreign process requires privileges that aren't trivial to set up. I've prepared the terrain, but also implemented a fallback strategy that calls `ps` to be sure that crucial information like the command name is available for all processes.

Diffs (updated)
processcore/CMakeLists.txt e7c9263
processcore/Info.plist PRE-CREATION
processcore/helper.cpp d54c8e1
processcore/processes_darwin_p.cpp PRE-CREATION
processcore/processes_local_p.cpp 2bc123f
processui/ReniceDlg.cpp a97563f

Diff: <a href="https://git.reviewboard.kde.org/r/128909/diff/" title="https://git.reviewboard.kde.org/r/128909/diff/">https://git.reviewboard.kde.org/r/128909/diff/</a>

Testing
On OS X 10.9.5 with Frameworks 5.24.0 and Qt 5.6.1

File Attachments
Attach to Process widget in KDevelop
<a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png" title="https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png">https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811...</a>

Thanks,

René J.V. Bertin

Re: Review Request 128909: initial, minimal support for OS X

By =?utf-8?Q?Ren=C... at 09/17/2016 - 13:32

(Updated Sept. 17, 2016, 7:32 p.m.)

Review request for KDE Base Apps and KDE Software on Mac OS X.

Changes
Fail with a nice error when trying to send a signal to processes belonging to another user.

Repository: libksysguard

Description
This patch implements initial and rather minimal support for OS X, for now focussing purely on process information. That feature is justified as it is used by KDevelop in order to obtain the list of processes one can attach a debugger to.

Mac OS X is tricky because it requires special privileges in order to obtain certain types of information for any running process. For example, even obtaining the number of threads spawned by a foreign process requires privileges that aren't trivial to set up. I've prepared the terrain, but also implemented a fallback strategy that calls `ps` to be sure that crucial information like the command name is available for all processes.

Diffs (updated)
processcore/CMakeLists.txt e7c9263
processcore/Info.plist PRE-CREATION
processcore/helper.cpp d54c8e1
processcore/processes_darwin_p.cpp PRE-CREATION
processcore/processes_local_p.cpp 2bc123f
processui/ReniceDlg.cpp a97563f
processui/ksysguardprocesslist.cpp 44603de

Diff: <a href="https://git.reviewboard.kde.org/r/128909/diff/" title="https://git.reviewboard.kde.org/r/128909/diff/">https://git.reviewboard.kde.org/r/128909/diff/</a>

Testing
On OS X 10.9.5 with Frameworks 5.24.0 and Qt 5.6.1

File Attachments
Attach to Process widget in KDevelop
<a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png" title="https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png">https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811...</a>

Thanks,

René J.V. Bertin

Re: Review Request 128909: initial, minimal support for OS X

By =?utf-8?Q?Ren=C... at 09/22/2016 - 10:50

(Updated Sept. 22, 2016, 4:50 p.m.)

Review request for KDE Base Apps and KDE Software on Mac OS X.

Changes
Cleanup

Repository: libksysguard

Description
This patch implements initial and rather minimal support for OS X, for now focussing purely on process information. That feature is justified as it is used by KDevelop in order to obtain the list of processes one can attach a debugger to.

Mac OS X is tricky because it requires special privileges in order to obtain certain types of information for any running process. For example, even obtaining the number of threads spawned by a foreign process requires privileges that aren't trivial to set up. I've prepared the terrain, but also implemented a fallback strategy that calls `ps` to be sure that crucial information like the command name is available for all processes.

Diffs (updated)
processcore/CMakeLists.txt e7c9263
processcore/Info.plist PRE-CREATION
processcore/processes_darwin_p.cpp PRE-CREATION
processcore/processes_local_p.cpp 2bc123f
processui/ReniceDlg.cpp a97563f
processui/ksysguardprocesslist.cpp 44603de

Diff: <a href="https://git.reviewboard.kde.org/r/128909/diff/" title="https://git.reviewboard.kde.org/r/128909/diff/">https://git.reviewboard.kde.org/r/128909/diff/</a>

Testing
On OS X 10.9.5 with Frameworks 5.24.0 and Qt 5.6.1

File Attachments
Attach to Process widget in KDevelop
<a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png" title="https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png">https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811...</a>

Thanks,

René J.V. Bertin

Re: Review Request 128909: initial, minimal support for OS X

By =?utf-8?Q?Ren=C... at 09/22/2016 - 15:25

(Updated Sept. 22, 2016, 9:25 p.m.)

Review request for KDE Base Apps and KDE Software on Mac OS X.

Changes
Hope I didn't introduce any more "wrong indentation" issues resolving the ones in ReniceDlg.cpp, or introduced "separate ticket" issues because the file isn't exactly consistent in its indentation...

Repository: libksysguard

Description
This patch implements initial and rather minimal support for OS X, for now focussing purely on process information. That feature is justified as it is used by KDevelop in order to obtain the list of processes one can attach a debugger to.

Mac OS X is tricky because it requires special privileges in order to obtain certain types of information for any running process. For example, even obtaining the number of threads spawned by a foreign process requires privileges that aren't trivial to set up. I've prepared the terrain, but also implemented a fallback strategy that calls `ps` to be sure that crucial information like the command name is available for all processes.

Diffs (updated)
processcore/CMakeLists.txt e7c9263
processcore/Info.plist PRE-CREATION
processcore/processes_darwin_p.cpp PRE-CREATION
processcore/processes_local_p.cpp 2bc123f
processui/ReniceDlg.cpp a97563f
processui/ksysguardprocesslist.cpp 44603de

Diff: <a href="https://git.reviewboard.kde.org/r/128909/diff/" title="https://git.reviewboard.kde.org/r/128909/diff/">https://git.reviewboard.kde.org/r/128909/diff/</a>

Testing
On OS X 10.9.5 with Frameworks 5.24.0 and Qt 5.6.1

File Attachments
Attach to Process widget in KDevelop
<a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png" title="https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png">https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811...</a>

Thanks,

René J.V. Bertin

Re: Review Request 128909: initial, minimal support for OS X

By Aleix Pol at 09/22/2016 - 12:05

processui/ReniceDlg.cpp (lines 56 - 57)
<https://git.reviewboard.kde.org/r/128909/#comment66908>

Wrong indentation

processui/ReniceDlg.cpp (line 79)
<https://git.reviewboard.kde.org/r/128909/#comment66909>

Wrong indentation.
Also isn't it possible to query the backend or something?

- Aleix Pol Gonzalez

On Sept. 22, 2016, 4:50 p.m., René J.V. Bertin wrote:

Re: Review Request 128909: initial, minimal support for OS X

By =?utf-8?Q?Ren=C... at 09/22/2016 - 13:30

Wrong indentation indeed, the file uses tabs for some reason.

Which backend do you want to query, for what? The "process backend" has no provision for indicating whether or not the host support changing anything, except for the IO priority. And I don't see how you could query the KAuth backend to know if these features should be disabled.

- René J.V.

On Sept. 22, 2016, 4:50 p.m., René J.V. Bertin wrote:

Re: Review Request 128909: initial, minimal support for OS X

By =?utf-8?Q?Ren=C... at 09/17/2016 - 15:52

processui/ReniceDlg.cpp (lines 78 - 87)
<https://git.reviewboard.kde.org/r/128909/#comment66826>

I've actually managed to get ksysguardprocesslist_helper to work with a patched libdbus and a hack to export the session bus socket name to the system dbus.

- René J.V. Bertin

On Sept. 17, 2016, 7:32 p.m., René J.V. Bertin wrote:

Re: Review Request 128909: initial, minimal support for OS X

By =?utf-8?Q?Ren=C... at 09/22/2016 - 10:50

I now have a much better patch for [lib]dbus which I hope to submit in a near future, after more extensive testing. In the meantime I'm leaving the libksysguard features disabled that require elevated privileges.

- René J.V.

On Sept. 17, 2016, 7:32 p.m., René J.V. Bertin wrote:

Re: Review Request 128909: initial, minimal support for OS X

By =?utf-8?Q?Ren=C... at 09/18/2016 - 20:49

I've been discussing this on the DBus ML, and am slowly getting my head about the hairy details and confusing terminology. I now know that the KAuth helper (DBus service executable, ksysguardprocesslist_helper in this case) has to connect on the system dbus. That removes the need for a hack, basically all I have to do is set `DBUS_LAUNCHD_SESSION_BUS_SOCKET` to the system dbus socket in the root user's environment.

It also means the libdbus patch can actually be discussed for upstreaming; the reason this particular feature doesn't work on OS X seems to be that helpers (services) like ksysguardprocesslist_helper don't even attempt to connect on the system bus on OS X. They just bail out when they fail to read the session bus socket name.

If we really wanted to tackle this problem correctly we'd write a specific (native) helper backend for KAuth, probably based on Apple's Service Management framework (which leverages launchd). I doubt that's worth the trouble at this point.

- René J.V.

On Sept. 17, 2016, 7:32 p.m., René J.V. Bertin wrote:

Re: Review Request 128909: initial, minimal support for OS X

By Aleix Pol at 09/18/2016 - 18:54

Is it really a good idea to rely on a patched libdbus?

- Aleix

On Sept. 17, 2016, 7:32 p.m., René J.V. Bertin wrote:

Re: Review Request 128909: initial, minimal support for OS X

By Aleix Pol at 09/15/2016 - 08:48

+1

processcore/processes_local_p.cpp (line 34)
<https://git.reviewboard.kde.org/r/128909/#comment66802>

drop trailing space

- Aleix Pol Gonzalez

On Sept. 15, 2016, 2:47 p.m., René J.V. Bertin wrote:

Re: Review Request 128909: initial, minimal support for OS X

By Aleix Pol at 09/14/2016 - 19:29

Other than that, the patch LGTM.

processcore/helper.cpp (line 133)
<https://git.reviewboard.kde.org/r/128909/#comment66774>

`KAUTH_HELPER_MAIN` doesn't work on OS X?

- Aleix Pol Gonzalez

On Sept. 14, 2016, 9:37 p.m., René J.V. Bertin wrote:

Re: Review Request 128909: initial, minimal support for OS X

By =?utf-8?Q?Ren=C... at 09/15/2016 - 13:44

FWIW:

```
If the current design depends on ksysguardprocesslist_helper being *launched* as root, the above error suggests that won't work on OS X (AFAIU it means the process won't find the DBus session daemon).

- René J.V.

On Sept. 15, 2016, 2:47 p.m., René J.V. Bertin wrote:

Re: Review Request 128909: initial, minimal support for OS X

By =?utf-8?Q?Ren=C... at 09/15/2016 - 12:55

Well ... it turns out I had never "back"ported my modifications to KDE4's Auth Services to KAuth, nor had anybody else in the 2 years since they were committed.

<a href="https://git.reviewboard.kde.org/r/126086/" title="https://git.reviewboard.kde.org/r/126086/">https://git.reviewboard.kde.org/r/126086/</a>

However, that doesn't fix everything. I now have a ksysguard app that obtains authorisation to do whatever it is it wants to do, and that then raises a DBus connection error because it is designed to use a helper for that purpose, which apparently fails to connect (I think it isn't even launched).

It seems ksysguard was never designed with platforms in mind where authorisation isn't obtained through a helper that's controlled via DBus. Technically that *should* work on OS X, but in practice KAuth would probably better do without.

What's the better approach here? Modify the ksysguard helper app so that it's started as a regular helper app and then obtains authorisation on its own? Or rewrite ksysguard so that it doesn't use a helper at all?

IMHO, the interest here is much more in providing an example for other applications (and me learning a few new tricks) rather than getting niceness control to work on OS X.
BTW, the scheduler changes should be disabled; can that be done independently from the niceness control?

- René J.V.

On Sept. 15, 2016, 2:47 p.m., René J.V. Bertin wrote:

Re: Review Request 128909: initial, minimal support for OS X

By =?utf-8?Q?Ren=C... at 09/15/2016 - 06:52

Not really related, but yeah I guess you're right.

FWIW, on OS X I see this:

```
That's at least 1 thing that doesn't work as it should; not yet sure what this has to do with the failures I'm seeing (the socket error is platform-agnostic so the KAuth OS X code does cross-thread stuff the Linux code apparently doesn't ... unless it's libdbus that uses threading)
).

- René J.V.

On Sept. 14, 2016, 9:37 p.m., René J.V. Bertin wrote:

Re: Review Request 128909: initial, minimal support for OS X

By Aleix Pol at 09/15/2016 - 06:04

I suggest dropping the workaround for now and investigate how to get KAuth to work altogether then.

- Aleix

On Sept. 14, 2016, 9:37 p.m., René J.V. Bertin wrote:

Re: Review Request 128909: initial, minimal support for OS X

By =?utf-8?Q?Ren=C... at 09/15/2016 - 04:59

FWIW, after I uninstalled the KDE4 helper app from my Linux system (`apt-get remove ksysguard`) I started getting the same error as I see on OS X when I try to increase a process's priority:

```
kf5.kauth: Tried to start an invalid action
kf5.kauth: Tried to start an invalid action
```

After reinstalling the KDE4 ksysguard package all errors went away and the feature works. Go figure ...
(I don't have KDE4 ksysguard installed on OS X)

- René J.V.

On Sept. 14, 2016, 9:37 p.m., René J.V. Bertin wrote:

Re: Review Request 128909: initial, minimal support for OS X

By =?utf-8?Q?Ren=C... at 09/15/2016 - 04:17

Aleix, you coined the idea of implementing a simpler widget that provides just a list of PIDs with their command names and possible command lines, based on the `ps` command (and comparable to the one in Qt Creator). Do you still think it'd be a good idea to add such a feature to libksysguard?
(Here's as good a place to raise the idea as any)

Thanks for picking up on this, I'd forgotten to make a comment about it.

I think the macro does work, it's KAuth which currently doesn't seem to work for me. It also doesn't in my Linux build, btw, but there at least it gives some error output suggesting tries to use polkit and possibly even the processlist helper from my KDE4 desktop.

The explicit implementation on OS X wasn't because of the macro, it was to allow running setuid root. Initial testing didn't show any benefit to that, but it could well be that's because KAuth still fails somewhere and then simply aborts the requested operation.

Suggestions very welcome, but if we can't get this to work I'll probably want to disable changing process priorities.

- René J.V.

On Sept. 14, 2016, 9:37 p.m., René J.V. Bertin wrote:

Re: Review Request 128909: initial, minimal support for OS X

By Anthony Fieroni at 09/14/2016 - 13:32

processcore/processes_darwin_p.cpp (line 104)
<https://git.reviewboard.kde.org/r/128909/#comment66771>

*inline* must be used in function definition not in declaration.

- Anthony Fieroni

On Септ. 14, 2016, 6:28 след обяд, René J.V. Bertin wrote:

Re: Review Request 128909: initial, minimal support for OS X

By =?utf-8?Q?Ren=C... at 09/14/2016 - 13:58

I didn't see the point of the way the keyword is being used here either, but I aligned myself to the current practice.

So I take it I shouldn't only correct `argMax()`?

- René J.V.

On Sept. 14, 2016, 5:28 p.m., René J.V. Bertin wrote:

Re: Review Request 128909: initial, minimal support for OS X

By Thiago Macieira at 09/15/2016 - 00:22

On quarta-feira, 14 de setembro de 2016 17:58:26 PDT René J.V. Bertin wrote:
It's all in the same .cpp, it doesn't matter. Inline is correct where it is.