D20007: Add GetProcessList for retrieving the list of currently active processes

[David Edmundson]

davidedmundson added a comment.


  To give a context for people who haven't seen the prior conversation.
  
  Right now kdevelop (optionally) pulls in libksysguard from plasma to show a simple process list, which isn't ideal. 
  We're not technically API stable and we'll go to Qt6 at a different time.
  
  A patch wants to add this in solid (and originally wanted to pull in libksysguard) which I've rejected because of the complicated dependency.
  
  We also have 2 other uses in plasma that use libksysguard just to get the name of a PID.
  
  I don't want to make libksysguard a framework as it's way too heavy for these usecases, it reads a billion /proc files populating a whole table full of stats and comes with a widgets dependency.
  
  IMHO a simpler portable process list of PID + name will be useful

INLINE COMMENTS

> hallas wrote in CMakeLists.txt:246
> Should util/kprocesslist.h also be added here?

yes

> hallas wrote in kprocesslist.h:1
> Is this license ok?

I think so, someone who's into licenses probably should check

> hallas wrote in kprocesslist.h:36
> I modified the interface slightly so that it fits better with other KDE APIs, I don't know what you guys think about it?

Generally fine, I would put the method in a namespace

> kprocesslist.h:41
> + */
> +struct KProcessInfo {
> +    unsigned int pid; ///< The pid of the process

This isn't future proof.

We probably need something with Pimpl (maybe a implicitly shared class)

> hallas wrote in kprocesslist.h:42
> I changed the pid to be a unsigned int instead, since both Windows and Unix use a number as the pid

Needs to be qint64 to cover windows

> hallas wrote in kprocesslist.h:43
> What is the exact semantics of this field?

Reading the code back this is:

the full command line if available (full /proc/$pid/cmdline)
 failing that the executable name (/proc/$pid/stat) 
 failing that the executable name (from ps)

(I have no idea about windows)

> hallas wrote in kprocesslist.h:45
> Do we really need this? Should we document all the valid states?

Definitely needs some docs. An enum might be better when we figure out what we want.

(or maybe just kill it?)

> hallas wrote in kprocesslist.h:56
> Is this the interface we want?

Based on the use cases I've seen I think we want:

KProcessList GetProcessList(); <-- for the kdevelop etc case

KProcess GetProcess(qint64 pid); <-- for the device manager / plasma vaults case

> hallas wrote in kprocesslist.h:56
> I haven't written any tests for the code as I am not sure how I can do that in a portable and reproduceable way.

As for unit tests, maybe something like:

- spawn an app with qprocess
- find it in the list

We'll know the PID from qprocess, we know our own user id, we know the name

REPOSITORY
  R244 KCoreAddons

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

To: hallas, davidedmundson, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190323/5839190b/attachment.html>