Add GetProcessList for retrieving the list of currently active
processes. This code is taken from:
https://github.com/KDAB/GammaRay/blob/master/launcher/ui/processlist.h
and modified slightly to better suit the needs of KDE.
Details
Details
Diff Detail
Diff Detail
- Repository
- R244 KCoreAddons
- Branch
- adds_kprocesslist (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 10869 Build 10887: arc lint + arc unit
| src/lib/CMakeLists.txt | ||
|---|---|---|
| 249 | Should util/kprocesslist.h also be added here? | |
| src/lib/util/kprocesslist.h | ||
| 2 | Is this license ok? | |
| 37 | I modified the interface slightly so that it fits better with other KDE APIs, I don't know what you guys think about it? | |
| 43 | I changed the pid to be a unsigned int instead, since both Windows and Unix use a number as the pid | |
| 44 | What is the exact semantics of this field? | |
| 45 | This appears to be a Windows only field, should we really keep this? | |
| 46 | Do we really need this? Should we document all the valid states? | |
| 57 | Is this the interface we want? | |
| 57 | 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. | |
| src/lib/util/kprocesslist_win.cpp | ||
| 2 | I do not have access to a Windows PC so I haven't built the Windows version | |
Comment Actions
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 (pulling in libksysguard) which I've rejected because of the complicated dependency loop.
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
| src/lib/CMakeLists.txt | ||
|---|---|---|
| 249 | yes | |
| src/lib/util/kprocesslist.h | ||
| 2 | I think so, someone who's into licenses probably should check | |
| 37 | Generally fine, I would put the method in a namespace | |
| 42 | This isn't future proof. We probably need something with Pimpl (maybe a implicitly shared class) | |
| 43 | Needs to be qint64 to cover windows | |
| 44 | Reading the code back this is: the full command line if available (full /proc/$pid/cmdline) (I have no idea about windows) | |
| 46 | Definitely needs some docs. An enum might be better when we figure out what we want. (or maybe just kill it?) | |
| 57 | 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 | |
| 57 | As for unit tests, maybe something like:
We'll know the PID from qprocess, we know our own user id, we know the name | |
| src/lib/util/kprocesslist.h | ||
|---|---|---|
| 57 | This method should follow the Qt naming style, i.e. processList(). | |
Comment Actions
Thanks a lot for the feedback guys, I will spend some time and implement it and push an updated patch.
Comment Actions
Ok, I have implemented all the review feedback you gave, I have also done some unit tests. Some of the unit tests or Unix specific though, I still don't know what to do about Windows since I don't have a development environment. Please give it a good look :D
Comment Actions
| src/lib/util/kprocesslist.cpp | ||
|---|---|---|
| 39 | we don't need a q_ptr It's only useful if the private object needs to signal things upwards. | |
| 66 | x = ProcessInfo() But you can simplify all this and the other one to *d_ptr = *other.d_ptr; | |
| 73 | delete d_ptr; OR replace make d_ptr a QScopedPointer OR make it a QSharedDataPointer and our KProcessInfoPrivate inherit from QSharedData | |
| src/lib/util/kprocesslist.h | ||
| 52 | qint64 pid, const QString &name, const QString &user it saves a string shallow copy | |
| src/lib/util/kprocesslist_p.h | ||
| 49–50 | We should probably either kill these 2 or use them. | |
Comment Actions
Config: Using QtTest library 5.12.1, Qt 5.12.1 (x86_64-little_endian-lp64 shared (dynamic) release build; by Clang 6.0.1 (tags/RELEASE_601/final 335540))
PASS : KProcessListTest::initTestCase()
PASS : KProcessListTest::testKProcessInfoConstructionAssignment() FAIL! : KProcessListTest::testProcessInfoList() 'processInfoList.empty() == false' returned FALSE. () Loc: [/home/adridg/src/kde/tier-1/kcoreaddons/autotests/kprocesslisttest.cpp(89)] FAIL! : KProcessListTest::testProcessInfo() 'processInfo.isValid() == true' returned FALSE. () Loc: [/home/adridg/src/kde/tier-1/kcoreaddons/autotests/kprocesslisttest.cpp(108)] PASS : KProcessListTest::testProcessInfoNotFound() PASS : KProcessListTest::cleanupTestCase() Totals: 4 passed, 2 failed, 0 skipped, 0 blacklisted, 1ms
I'll chase this for 34 minutes now (until 2pm)
Comment Actions
On FreeBSD, /proc is not necessarily mounted (it might be a Linuxism). So while I do have /proc, it's empty because procfs isn't mounted there. If I do mount it, then there's the expected list of processes and a curproc symlink. But /proc/<pid> doesn't contain a stat file .. there's a status, though. Let me mess around a bit with that... yes, changing to status makes all 6 tests pass. So I'd suggest something like
#ifdef Q_OS_FREEBSD
QString statusFileName(QStringLiteral("/status"));
#else
QString statusFileName(QStringLiteral("/stat"));
#endifand then later on
filename += statusFileName;
Comment Actions
Don't forget to update the commit message (it still mentions "Add GetProcessList") ;)
Comment Actions
@adridg - thanks a lot for testing the patch on FreeBSD! I have incorporated the changes you suggested, it would be nice if you could give it a test again :D
Comment Actions
BTW - I tried locally chaning the D19989 patch to use the new KProcessList::processInfoList function and that enables us to achieve the same thing but without the KSysGuard dependency, so that's great :)
Another thing, should we try and generalize the lsof functionality? And if so, where should it go?
Comment Actions
@davidedmundson - could you please take a look at this again? I have implemented all the stuff you suggested ;)
Comment Actions
Looks good to me. \o/
Hopefully a windows person can test soon.
Failing that, it's near the start of the month and we have a good unit test. We can ship it and see what CI says.
| src/lib/util/kprocesslist.cpp | ||
|---|---|---|
| 70 | With the QSharedDataPointer you can delete this method. The point of the shareddatapointer is that when you copy KProcessInfo you don't copy all the members, we just increase the internal ref-count | |
| src/lib/util/kprocesslist.cpp | ||
|---|---|---|
| 70 | If I delete the method I get compilation errors, since users of this class doesn't know the definition of KProcessInfoPrivate - so the method needs to be here, it could just be a whole lot simpler ;) | |
Comment Actions
The CI isn't happy with this code.
FreeBSD fails: https://build.kde.org/job/Frameworks/job/kcoreaddons/job/kf5-qt5%20FreeBSDQt5.13/lastCompletedBuild/testReport/projectroot/autotests/kprocesslisttest/
Windows fails: https://build.kde.org/job/Frameworks/job/kcoreaddons/job/kf5-qt5%20WindowsMSVCQt5.11/95/testReport/junit/projectroot/autotests/kprocesslisttest/
Please take a look.